[PropertyAccess] Fix checking for missing properties

[nicolas-grekas]

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #48970, fix #53972, fix #51919
License	MIT

PropertyAccess and PropertyInfo make a few assumptions when magic methods are involved.
Namely they assume that if a __get method is defined, then any property name is accessible.
This assumption is generally wrong, and this becomes more evident with the introduction of lazy objects.
The linked issue gives some more details.

In this PR, I tweak these assumptions in order to make less of them.
Note that there is no bullet-proof way to decide if a virtual property exists in PHP. We're missing an __exists() magic method for that. Because of this, I'm submitting this PR to 6.4 and not 5.4. Let 5.4 end its life quietly and ensure 6.4 works at its best with lazy ghosts, where they're mainstream.

[chalasr]

Is there a way to test this?

[nicolas-grekas]

I think I did add tests, do you have something more in mind?

[chalasr]

Something must have blinded me 👀 Sorry!

[chalasr]

Is the __exists() your next RFC?

[nicolas-grekas]

I don't think so :)

[alexandre-le-borgne]

I tested this fix but it doesn't work for me

I use PhpDocExtractor to be able to denormalize objects with properties like this:

class Form // Not a doctrine entity
{
    /**
     * @var Step[]
     */
    private array $steps;
}

class Step // Not a doctrine entity
{
}

However I still get the same error when I serialize this kind objects :

class FormOutput // Not a doctrine entity
{
    /**
     * @var IndicatorType                                    <--- A Doctrine entity
     */
    private IndicatorType $indicatorType;
}

User Notice: Undefined property: Proxies_CG_\App\Entity\IndicatorType::$lazyObjectState in /home/.../src/Serializer/PropertyAccessor.php on line 419

[xabbuh]

@alexandre-le-borgne Can you please open a new issue and provide a small example application that allows to reproduce your issue?

[alexandre-le-borgne]

@xabbuh #54393

[sbuerk]

Is there a reason why the $access->canBeReference() check has been dropped here ?

[nicolas-grekas]

yes: that check was just wrong - __get can return by reference, so this was an unrelated check

[sbuerk]

Thanks for the information.

[vsoroka]

I guess that a better sollution will be to create an interface for classes that will ckeck existance of magic properties, e.g. MagicPropertyExistanceExtractorInterface, and if Symfony\Component\PropertyInfo\Extractor\ReflectionExtractor implements this interface delegate the existance check to it. It will allow to implement application depended logic related to magic properties without workarounds.
Unfortutattely since version 6.4.6 I have to configure the property accessor to ignore invalid properties in my application and try to find a way to do this check outside the property accessor.

[craigh]

This change broke my tests. They work with 6.4.4 and not with 6.4.6.
I get the error Can\'t get a way to read the property "%s" in class "%s".'

I use magic getters in one of my entities like so:

    public function __get(string $arg)
    {
        if (preg_match('/custom_checkbox_(\d+)_entry(_other)?/', $arg, $matches)) {
            if (isset($matches[2])) {
                return $this->getCustomCheckboxEntryOther((int) $matches[1]);
            }

            return $this->getCustomCheckboxEntryForCheckbox((int) $matches[1]);
        }
        if (preg_match('/customCheckboxEntryValueByCheckboxId(\d+)/', $arg, $matches)) {
            return $this->getCustomCheckboxEntryValueByCheckboxId((int) $matches[1]);
        }
    }

my tests use liip/test-fixtures-bundle (if this matters).

[nicolas-grekas]

@craigh you might have a broken implementation of __isset, please have a look and try to fix it.

[craigh]

@nicolas-grekas of course, you are correct 🎉 thank you for your keen insight and quick response!

[fmonts]

This is a BC break, I'm now getting:

[NoSuchPropertyException]
Can't get a way to read the property "offerFallback" in class "App\Entity\Offer".

Even if I have this, to access dynamic properties stored in a json column:

public function __get(string $name): mixed
{
    if (property_exists(Offer::class, $name)) {
        return $this->$name;
    }
    return $this->getSetting($name) ?? null;
}

It's look like the magic getter is never called

[nicolas-grekas]

@fmonts same as above: you likely have a broken implementation of __isset, please have a look and try to fix it.

[fmonts]

@fmonts same as above: you likely have a broken implementation of __isset, please have a look and try to fix it.

I don't have any __isset(), I accept virtually any name, this would require me to add

public function __isset(string $name): bool
{
    return true;
}

to many entities...

[nicolas-grekas]

That's why then. You relied on a specific behavior of PropertyAccess, which is broken in the generic case (eg the ?? operator should be broken with you objects, etc).

[barryvdh]

This seems to break access to properties in Laravel Eloquent models when the value is null. This is because __isset() returns false if the value is null and no actual property exists on the model. And if a model is not loaded from the database yet, it will have no properties. So it's impossible to check if it will be allowed.

I'm not sure if there is a 'standard' way to check for access there, because of the way ActiveRecord works (at least in Laravel). In Laravel it is always possible to set or get a property on the model..
Eg. when setting $model->name = 'Foo'; it will set $model->attributes['name'] (and perform additional actions when setting/getting) using the magic __get() and __set() methods. If an attribute is requested that does not exist, it will return null.