Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark nonexistent properties from __debugInfo() in var_dump #16852

Open
kkmuffme opened this issue Nov 18, 2024 · 5 comments
Open

Mark nonexistent properties from __debugInfo() in var_dump #16852

kkmuffme opened this issue Nov 18, 2024 · 5 comments

Comments

@kkmuffme
Copy link

Description

Essentially #16793 also exists with PHP native classes as pointed out in #16793 (comment)

The following code:

https://3v4l.org/ZS1MZ#v8.3.13

<?php

$dt = new DateTime("now");
var_dump($dt);
var_dump(property_exists($dt, "timezone"));
var_dump($dt->timezone);

Resulted in this output:

object(DateTime)#1 (3) {
  ["date"]=>
  string(26) "2024-11-18 23:33:30.008856"
  ["timezone_type"]=>
  int(3)
  ["timezone"]=>
  string(16) "Europe/Amsterdam"
}
bool(false)

Warning: Undefined property: DateTime::$timezone in /in/ZS1MZ on line 6
NULL

But I expected this output instead:
Either the timezone property should be marked as private or property_exists and direct access to it shouldn't fail

I assume this behavior is the result of https://www.php.net/manual/en/language.oop5.magic.php#language.oop5.magic.debuginfo - however I think this should be marked in the dump, since this is unexpected at least. Since this is applied in PHP Error/Exception stack traces too, this can definitely be misleading.
Possibly there should be a way to disable that for stack traces?

PHP Version

8.3

Operating System

No response

@iluuu1994
Copy link
Member

iluuu1994 commented Nov 18, 2024

Hi @kkmuffme! Yeah, still would still not consider this a bug. var_dump() of objects does not necessarily dump properties. They usually are, but not when the debug output is customized. You can even do this yourself in __debugInfo(). These two are roughly equivalent. https://3v4l.org/Prc8n

Either the timezone property should be marked as private or property_exists and direct access to it shouldn't fail

As mentioned, there is no such property.

I assume this behavior is the result of https://www.php.net/manual/en/language.oop5.magic.php#language.oop5.magic.debuginfo - however I think this should be marked in the dump, since this is unexpected at least. Since this is applied in PHP Error/Exception stack traces too, this can definitely be misleading.
Possibly there should be a way to disable that for stack traces?

We can treat this part as a feature request. However, it's worth noting that the output of var_dump() is considered at least semi-stable. Modifying it might be a tough sell. Furthermore, it may also be additionally misleading. E.g. what property gets marked? Only the ones that don't exist? Or also the ones that don't hold the same value as the actual property? What about properties that are implemented using hooks or __isset/__get? Lots of corner cases. IMO, this is not a problem worth solving.

@iluuu1994 iluuu1994 changed the title var_dump includes non-existing (but somehow existing?) properties DateTime Mark nonexistent properties from __debugInfo() in var_dump Nov 18, 2024
@kkmuffme
Copy link
Author

kkmuffme commented Nov 19, 2024

The "bug" is that it's currently not possible to distinguish between public properties and properties set with __debugInfo - in var_dump they look the same.
I guess just like "protected" and "private" properties, all properties that are derived from debugInfo should be marked accordingly.

What about properties that are implemented using hooks or __isset/__get?

This is something completely different - these are not in var dump now either and that doesn't need to be changed.
The problem is the differentiation between actual public properties and properties that are only set with debugInfo.


Tangentially related: (how) is the asymmetric visibility in PHP 8.4 marked in var_dump?

@iluuu1994
Copy link
Member

iluuu1994 commented Nov 19, 2024

This is something completely different - these are not in var dump now either and that doesn't need to be changed.

You might have misunderstood what I meant. The feature request is that properties that don't exist should be marked accordingly, e.g. through some symbol. But what you're likely implying is that any property that cannot be accessed must be marked. It's possible to access properties without them being explicitly declared, e.g. through __get. So, should those be marked, when they are returned from __debugInfo()? https://3v4l.org/fnPNu Edit: Oh, actually property_exists() returns false for __isset() => true, I was not aware of that.

Tangentially related: (how) is the async visibility in PHP 8.4 marked in var_dump?

Assuming you mean asymmetric(ly visible) properties, The same as symmetric properties.

@kkmuffme
Copy link
Author

But what you're likely implying is that any property that cannot be accessed must be marked

I'm not implying anything :-) I literally mean: for properties currently output in var_dump the visibility should be correctly reported.
It is not currently correctly reported for anything from debugInfo, since it's reported as public.

Assuming you mean asymmetric(ly visible) properties, The same as symmetric properties.

Yes, I was working in another language while writing that issue.

I guess this would also need improving so the asymmetric visibility is reported correctly?

@iluuu1994
Copy link
Member

iluuu1994 commented Nov 19, 2024

I see your point, but as mentioned, once you're using __debugInfo, those hints really go out the window. For example:

class C {
    protected $prop = 'prop';

    public function __debugInfo() {
        return ['prop' => strtoupper($this->prop)];
    }
}
var_dump(new C());

This outputs:

object(C)#1 (1) {
  ["prop"]=>
  string(4) "PROP"
}

Without the __debugInfo(), it outputs:

object(C)#1 (1) {
  ["prop":protected]=>
  string(4) "prop"
}

Marking "prop" as :protected in the first case would be misleading, because it implies it is a property that holds the value "PROP", when that's not actually the case. Even if we were to do this, it wouldn't help for the case that you originally reported, because DateTime does not actually declare a property called $timezone. It only exports it via __debugInfo(). There is not even a private or protected property. Marking it as :nonexistent is still misleading for __get, as previously discussed. I don't think var_dump() is meant to provide an accurate representation of the object/class. That's why we have reflection.

I think at this point it makes more sense to move the discussion to a more public medium. If you like, you may send an e-mail to the internals mailing list to propose a concrete change, and ask for opinions on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants