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

ValueHolder proxy for class with accessing to private property within final method #472

Open
kim-skyeng opened this issue Jul 5, 2019 · 15 comments
Assignees
Milestone

Comments

@kim-skyeng
Copy link

kim-skyeng commented Jul 5, 2019

I have class with accessing to private property within final method like this:

class ClassWithFinalMethodAndPrivateProperty
{
    private $prop = 'privet';

    final public function finalMethod()
    {
        return $this->prop;
    }
}

I get Cannot access private property $prop error if I call finalMethod on valueHolder proxy of this class.

Is there such limitation for Value HolderProxy using or did I something wrong? Can't find any mentions about it in documentation or issues.

@Ocramius
Copy link
Owner

Ocramius commented Jul 5, 2019

Could you make an example of something triggering this, in the context of ProxyManager?

@kim-skyeng
Copy link
Author

kim-skyeng commented Jul 5, 2019

I tried launch this code. It triggers error.

<?php

namespace App;

use ProxyManager\Factory\LazyLoadingValueHolderFactory;
use ProxyManager\Proxy\LazyLoadingInterface;

require_once __DIR__ . '/vendor/autoload.php';

$factory = new LazyLoadingValueHolderFactory();
$initializer = function (& $wrappedObject, LazyLoadingInterface $proxy, $method, array $parameters, & $initializer) {
    $initializer = null; // disable initialization
    $wrappedObject = new ClassWithFinalMethodAndPrivateProperty(); // fill your object with values here

    return true; // confirm that initialization occurred correctly
};

$proxy = $factory->createProxy(ClassWithFinalMethodAndPrivateProperty::class, $initializer);

echo $proxy->finalMethod();

@Ocramius
Copy link
Owner

Ocramius commented Jul 5, 2019

Interesting. I don't think that's fixable (the final method cannot be replaced). Do you have a stack trace for this?

@kim-skyeng
Copy link
Author

kim-skyeng commented Jul 6, 2019

Trace:

PHP Fatal error:  Uncaught Error: Cannot access private property App\ClassWithFinalMethodAndPrivateProperty::$prop in ~/vendor/ocramius/proxy-manager/src/ProxyManager/GeneratorStrategy/EvaluatingGeneratorStrategy.php(70) : eval()'d code:96
Stack trace:
#0 ~/vendor/ocramius/proxy-manager/src/ProxyManager/GeneratorStrategy/EvaluatingGeneratorStrategy.php(70) : eval()'d code(101): ProxyManagerGeneratedProxy\__PM__\App\ClassWithFinalMethodAndPrivateProperty\Generated4fba263a26adebb7c3e20106c2b98b63->ProxyManagerGeneratedProxy\__PM__\App\ClassWithFinalMethodAndPrivateProperty\{closure}()
#1 ~/src/ClassWithFinalMethodAndPrivateProperty.php(11): ProxyManagerGeneratedProxy\__PM__\App\ClassWithFinalMethodAndPrivateProperty\Generated4fba263a26adebb7c3e20106c2b98b63->__get('prop')
#2 ~/src/a.php(20): App\ClassWithFinalMethodAndPrivateProperty->finalMethod()
#3 {main}

Next Error: Cannot access private property App\Class in ~/vendor/ocramius/proxy-manager/src/ProxyManager/GeneratorStrategy/EvaluatingGeneratorStrategy.php(70) : eval()'d code on line 96

I also tried to debug this code a bit and saw that php evaluate ClassWithFinalMethodAndPrivateProperty::finalMethod code with ProxyClass object as $this instead of call special inherited ClassWithFinalMethodAndPrivateProperty_123123::finalMethod like it does for non-final methods.

@Ocramius
Copy link
Owner

Ocramius commented Jul 6, 2019

Yeah, the issue here is that no initialisation can happen, so method calls cannot be redirected to the wrapped value.

@kim-skyeng
Copy link
Author

kim-skyeng commented Jul 6, 2019

So it's pretty expected behavior, and ProxyManager users just should keep in mind that ValueHolder does not really good work with final methods?

@Ocramius
Copy link
Owner

Ocramius commented Jul 6, 2019

Pretty much. I could alter the behaviour in a major release, so that it throws during code generation (since final methods aren't proxied).

See #187 and #189

@kim-skyeng
Copy link
Author

kim-skyeng commented Jul 6, 2019

Well I would like also to tell my background story of this case. Hope it could be helpfully for people who google this problem.

It happened when I use Symfony lazy service with final methods as described above. Symfony DI uses ValueHolder for lazy services so toggling off laziness fix the problem. Also code works fine when I declare property protected or make methods non final. Of course all final methods without accessing to private members could be evaluated too.

@Ocramius
Copy link
Owner

Ocramius commented Jul 6, 2019

A final method would be broken anyway, so I think that throwing an exception upfront is a wiser choice here...

@kim-skyeng
Copy link
Author

kim-skyeng commented Jul 6, 2019

Do you mean ValueHolder should not support classes with final methods even if it can work until accessing private members?

@Ocramius
Copy link
Owner

Ocramius commented Jul 6, 2019

Nono, it will be broken even in case of public state.

Try proxying this:

class Counter
{
    public $count = 0;
    final public function increment () : int
    {
        return $this->count += 1;
    }
    public function decrement () : int
    {
        return $this->count -= 1;
    }
}

$counter = makeProxy(Counter::class);

var_dump($counter->increment());
var_dump($counter->increment());
var_dump($counter->decrement());

This should print:

1
2
-1

@Ocramius
Copy link
Owner

Ocramius commented Jul 6, 2019

Actually, it may even work, now that I think of it (because of the __get tricks I wrote), but still very broken due to split method behaviour: one method acts on the state of the proxy, the other acts on the state of the wrapped instance.

@kim-skyeng
Copy link
Author

kim-skyeng commented Jul 6, 2019

Your code works okay.
Upd. I meant your code with using proxy

1
2
1

But I see what do you mean, there is could be more problems with final methods. So it should either be fully supported or throws exception during code generation.

@Ocramius
Copy link
Owner

Ocramius commented Jul 6, 2019

Let's re-open here to track the BC break to be introduced

@nicolas-grekas
Copy link
Contributor

This issue can be fixed by duplicating the logic that ghost objects use: unset all properties on the virtual proxy and forward all access to them to the target object.

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

3 participants