Unable to detect magic __get properties correctly registered with __isset

[tcarrio]

Description

This was specifically found when working with the column(...) method in the AbstractCollection implementation. The issue is that PHP's magic getter functionality is not supported.

Steps to reproduce

1. Write a class Example that defines no properties with a __get($key) and __isset($key) method to return "value" and true when the $key is "key", respectively
2. Make a new Collection of Examples, e.g. new Collection([new Example()])
3. Call $collection->column('key')
4. A ValueExtractionException is thrown

Expected behavior

An array is returned: ["value"]

Screenshots or output

Environment details

• version of this package: 1.3.0
• PHP version: 8.1
• OS: macOS

Additional context

Maybe I can get to a PR on this- but no current capacity for it

[SimoTod]

I think you just need to add

        if (method_exists($element, '__get')) {
            return $element->$propertyOrMethod;
        }

here to support it.

The tradeoff is that classes with a magic getter will never throw ValueExtractionException but just delegate the resolution to the getter itself (I'd say it's probably expected).

[tcarrio]

@SimoTod I may be wrong, but I understood best practice was to utilize both __isset as a way to dictate when __get would return a value. This feels safer to me than making all classes with __get return a value. I had thrown together a patch for this and could push a PR shortly with testing.

[SimoTod]

@SimoTod I may be wrong, but I understood best practice was to utilize both __isset as a way to dictate when __get would return a value. This feels safer to me than making all classes with __get return a value. I had thrown together a patch for this and could push a PR shortly with testing.

Sure, that's best practice but you need to document it in this case because it's not required.

You can have a magic __get in a standard php class and that would work when you call an arbitrary non existing property.

All class with get, would call the magic getter if defined and get, depending on the implementation, may as well throw an exception if not trying to access something not expected.

Slightly related, in theory, you could make the same reasoning for __call and methods.

[tcarrio]

Definitely could do a similar check for __call as well.

I'm not sure where this would best be documented; for example, the behavior today of not supporting magic methods isn't documented.

[tcarrio]

I opened that PR to do the checks with isset to support __get and __isset classes. We can discuss which path would be preferred going forward. I'd be interested in hearing more from @ramsey on it as well 🙏

[SimoTod]

Just to clarify the expected behaviour: With a fairly standard implementation (taken from the php documentation) you would not be able to access something set to null since isset is meant to return false in that case so __isset is usually (or at least sometimes) implemented in that way. See https://onlinephp.io/c/408a9

[tcarrio]

Yes the standard behavior of isset will return false when a value is null, but when implementing the __isset magic method behavior it's up to the code owner.

That method signature is public __isset(string $name): bool, and doesn't inherently rely on isset, it overrides it.

At the point in the code where we've already checked for explicit properties, and explicit methods, the case for properties accessible via __get is not defined- and the only consistent manner to dictate that it is accessible is via isset and the __isset magic method. Otherwise, like you given an example of, it will always return something, even if it's a bunch of nulls.

[SimoTod]

Not really, it's up to the code owner in the same way. Generally a magic get will contain some ifs, return the value if those conditions are satisfied or throw if not (same as the extractor class, just a different type of error but we could catch it and let the code throw the correct exception if we are concerned about consistency). The only defect that I can see is that you have to run get to know if it will error (and because you don't know about the implementation, you don't know what side effects it could have even if it shouldn't have any) so yeah, not ideal either and maybe the isset approach makes more sense.

However, we mentioned best practices above but then we are suggesting the code owner should implement it in a way that doesn't follow the original purpose of the method. If it's a third party library implemented in a canonical way (could be part of a dependency imported with composer), you won't have much control on it as well. https://www.php.net/manual/en/language.oop5.overloading.php

It feels like we really need a __property_exists magic (someone did try to ask for it at some point https://bugs.php.net/bug.php?id=44857 :)).