doctrine / common

Doctrine Common
https://www.doctrine-project.org/projects/common.html
MIT License
5.79k stars 294 forks source link

Proxies - Access to private property from the same class scope (recursive) does not initialise Proxy #934

Closed olsavmic closed 2 years ago

olsavmic commented 3 years ago

Recently I came across an issue that was hard to debug and quite unexpected to me.

Code like the one below, which is fully valid PHP code

class Foo {
    /**
      * @ORM\ManyToOne()
      */
    private Foo $parent;

    ... // Always initialize $this->parent in constructor

    public function doSomething(): void {
         var_dump($parent->parent);
    }
}

will result in Typed property accessed before initialization error.

The situation is even worse with nullable properties which will simply return NULL which is pretty hard to spot.


I understand that it's an internal limitation of how currently proxy classes work BUT support for public properties has been added in 2.4 if I recall correctly.

I was not able to find this issue documented anywhere so I'd either:

1) Add visible warning in the documentation to never access private properties directly from the same class. 2) Handle private/protected properties the same way as public properties are currently handled

Also, since I've seen plans to move from custom implementation of Proxies to https://github.com/Ocramius/ProxyManager (https://github.com/doctrine/orm/issues/8518), is it possible that this transition will resolve this issue?

stof commented 3 years ago

Also, since I've seen plans to move from custom implementation of Proxies to https://github.com/Ocramius/ProxyManager, is it possible that this transition will resolve this issue?

it will probably solve it, because ProxyManager hooks directly in property accesses instead of hooking into public method calls.

olsavmic commented 3 years ago

Great, thank you! Do we have any rough ETA for the transition? Perhaps the docs could be updated to reflect this issue until it's fixed. Or am I missing something and it's already there?

derrabus commented 3 years ago

Do we have any rough ETA for the transition?

It happens, when somebody makes it happen. I'm not aware that someone is actively working on that topic.

smilesrg commented 3 years ago

will result in Typed property accessed before initialization error.

But isn't that an expected behaviour?

beberlei commented 2 years ago

As @smilesrg mentions, accessing an unitialized property before initializing it is expected behavior.

olsavmic commented 2 years ago

@smilesrg @beberlei I don't think we understand each other, I probably did not make it clear in the example.

The $parent variable always contains data, it's a non-nullable column and it's always initialised in the constructor.

Sure, if the property was not really initialised, it's expected behaviour to throw the error. But the property does in fact contain data, the only issue is that the PROXY object is not initialised yet.

Accessing public property/method always initialises the proxy whereas private/protected properties/methods do not. The situation is even worse in case of nullable properties (no error is thrown) and setters -> the value will be overwritten during initialisation without any notice.

Very similar, although less likely usage is this:

class Foo {
    /** 
     * @ORM\Column(type="string", nullable=false)
    private string $name = 'abc';

    public function doSomething(self $instance): void {
        // throws in case $instance is a uninitialised proxy object
         var_dump($instance->name);
    }
}

Correct me if this is documented anywhere but I was not able to find any mention of something like "Accessing private/protected members of related entity of the same class is not recommended as the property may not contain data in case the instance is an uninitialised proxy object."

We created a workaround by calling every-time we're accessing private property on different object of the same class (checked by static analysis):

        if ($instance instanceof Proxy) {
            $instance->__load();
        }

That's fine for our use case BUT I'd like to at least improve the documentation because finding the cause of such errors was not straightforward and I think many people may encounter the same problem (unknowingly).

I also wanted to make sure that the issue is considered during the migration to ProxyManager (which should fix the issue) or future updates to current proxies.

Therefore I'd not close the issue at least until we update the docs (@beberlei).

olsavmic commented 2 years ago

I've created a proposal PR (https://github.com/doctrine/orm/pull/9311) pointing out the following issue in the docs. I'd appreciate feedback to let me know if it makes sense to you :)

Thanks!