Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC] [Vote] var_info »

[RFC] [Vote] var_info

php.internals

Fleshgrinder

10 years ago
Hi! Voting on var_info has opened and will stay open until 2016-07-22: https://wiki.php.net/rfc/var_info
-- Richard "Fleshgrinder" Fussenegger

Nikita Popov

10 years ago
On Fri, Jul 8, 2016 at 9:49 PM, Fleshgrinder <php@fleshgrinder.com> wrote:
> Hi! > > Voting on var_info has opened and will stay open until 2016-07-22: > > https://wiki.php.net/rfc/var_info >
Not a big fan of this in general (imho belongs in userland), but I'd like to comment on one specific issue: You are doing a callability check for all arrays and strings. I wouldn't be comfortable passing any data that potentially derives from external sources (aka all data) to that function. E.g. this could trigger autoloading on user-provided values -- while we have some protections in place to avoid the most egregious security vulnerabilities, I wouldn't be comfortable with that. There probably other funky things that can happen here. Lastly, I'll just leave this list of type-related improvements I'd actually like to see: * Make is_object() return true for IncompleteClass. This is just ridiculous. I care zero about the theoretical BC impact. * Make gettype() return 'resource (unknown type)' instead of just 'unknown type' for closed resources. This makes the output clear while still leaving the closed resource distinction. * Normalize error message. IIRC currently our type error messages are really weird, in that they use terms like "integer" which are not actually valid type hints (or have different meaning). Fixing this doesn't even need a proposal, just a PR. Regards, Nikita

Stas Malyshev

10 years ago
Hi!
> Lastly, I'll just leave this list of type-related improvements I'd actually > like to see: > * Make is_object() return true for IncompleteClass. This is just > ridiculous. I care zero about the theoretical BC impact.
I have no idea why it doesn't do so. I'll try to dig up when it was made so and see if there's some explanation.
> * Make gettype() return 'resource (unknown type)' instead of just > 'unknown type' for closed resources. This makes the output clear while > still leaving the closed resource distinction.
That makes total sense. And BC impact is rather minimal given that closed resources are useless.
> * Normalize error message. IIRC currently our type error messages are > really weird, in that they use terms like "integer" which are not actually > valid type hints (or have different meaning). Fixing this doesn't even need > a proposal, just a PR.
Right. But ensure tests are fixed, that's probably where one has to spend some time on cleaning it up.
-- Stas Malyshev smalyshev@gmail.com

Stas Malyshev

10 years ago
Hi!
>> * Make is_object() return true for IncompleteClass. This is just >> ridiculous. I care zero about the theoretical BC impact. > > I have no idea why it doesn't do so. I'll try to dig up when it was made > so and see if there's some explanation.
This is the change that caused it: commit 7b3fb771e4b022a8bf74b1a881dbec67d3ad460b Author: Yasuo Ohgaki <yohgaki@php.net> Date: Wed Jul 24 09:55:11 2002 +0000 is_object() returns FALSE if object is a "incomplete object". Raise E_NOTICE, instead of E_ERROR, for setting/getting properties to/from a "incomplete object". There's some discussion on this patch: http://grokbase.com/t/php/php-dev/027rwwmc8k/php-cvs-cvs-php4-ext-standard-incomplete-class-c-php-incomplete-class-h-type-c There are references to previous discussions but I couldn't find it. Yasuo, I realize it was a long time ago, but maybe you remember why it was done and can weight in on this?
-- Stas Malyshev smalyshev@gmail.com

Nikita Popov

10 years ago
On Sat, Jul 9, 2016 at 12:19 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
> Hi! > > > Lastly, I'll just leave this list of type-related improvements I'd > actually > > like to see: > > * Make is_object() return true for IncompleteClass. This is just > > ridiculous. I care zero about the theoretical BC impact. > > I have no idea why it doesn't do so. I'll try to dig up when it was made > so and see if there's some explanation. > > > * Make gettype() return 'resource (unknown type)' instead of just > > 'unknown type' for closed resources. This makes the output clear while > > still leaving the closed resource distinction. > > That makes total sense. And BC impact is rather minimal given that > closed resources are useless. >
I've created PRs for both of these: gettype() on closed resources: https://github.com/php/php-src/pull/2022 is_object() on Incomplete_Class: https://github.com/php/php-src/pull/2023 Unless there are objections from core devs I'll apply both in a few days. Nikita

Nikita Popov

10 years ago
On Fri, Jul 22, 2016 at 10:21 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:
> On Sat, Jul 9, 2016 at 12:19 AM, Stanislav Malyshev <smalyshev@gmail.com> > wrote: > >> Hi! >> >> > Lastly, I'll just leave this list of type-related improvements I'd >> actually >> > like to see: >> > * Make is_object() return true for IncompleteClass. This is just >> > ridiculous. I care zero about the theoretical BC impact. >> >> I have no idea why it doesn't do so. I'll try to dig up when it was made >> so and see if there's some explanation. >> >> > * Make gettype() return 'resource (unknown type)' instead of just >> > 'unknown type' for closed resources. This makes the output clear while >> > still leaving the closed resource distinction. >> >> That makes total sense. And BC impact is rather minimal given that >> closed resources are useless. >> > > I've created PRs for both of these: > > gettype() on closed resources: https://github.com/php/php-src/pull/2022 > is_object() on Incomplete_Class: https://github.com/php/php-src/pull/2023 > > Unless there are objections from core devs I'll apply both in a few days. > > Nikita >
As there were no objections, I've now merged both changes: https://github.com/php/php-src/commit/34824b70f8e72b200d77957145bb61883b03322d https://github.com/php/php-src/commit/89f63779711f34d87c2464ae13965df868d6231f As I forgot to handle this earlier, I went with master-only. Nikita

Fleshgrinder

10 years ago
Many thanks for the feedback to both of you. :) On 7/8/2016 11:54 PM, Nikita Popov wrote:
> You are doing a callability check for all arrays and strings. I wouldn't be > comfortable passing any data that potentially derives from external sources > (aka all data) to that function. E.g. this could trigger autoloading on > user-provided values -- while we have some protections in place to avoid > the most egregious security vulnerabilities, I wouldn't be comfortable with > that. There probably other funky things that can happen here. >
That is true for anything that uses is_callable anywhere. On 7/8/2016 11:54 PM, Nikita Popov wrote:
> Lastly, I'll just leave this list of type-related improvements I'd actually > like to see: > * Make is_object() return true for IncompleteClass. This is just > ridiculous. I care zero about the theoretical BC impact.
I figured that this situation is on purpose and I don't think that it is ridiculous per se. It makes sense if is_object() is defined as only identifying valid objects (and incomplete class is by definition invalid). On 7/8/2016 11:54 PM, Nikita Popov wrote:
> * Make gettype() return 'resource (unknown type)' instead of just > 'unknown type' for closed resources. This makes the output clear while > still leaving the closed resource distinction.
Will prepare a PR. 8) On 7/8/2016 11:54 PM, Nikita Popov wrote:
> * Normalize error message. IIRC currently our type error messages are > really weird, in that they use terms like "integer" which are not actually > valid type hints (or have different meaning). Fixing this doesn't even need > a proposal, just a PR. >
Definitely on my roadmap anyways and yes stas, this requires going through a sh**load of existing tests but it is definitely worth it. I will most probably start work on all of this this weekend after being on vacation for the last days.
-- Richard "Fleshgrinder" Fussenegger