Fixes #896: Undefined variable: name in generated Proxy

[miloshavlicek]

Renaming __get(), __set() and __isset() parameters is safer (due to potential conflicts with existing var names)

Related to: issue #868 and PR #869

[greg0ire]

@flack please test this

@miloshavlicek please help @flack by running https://gist.github.com/greg0ire/a404831add1247d2bc20fa11107c5d5e from the directory where you cloned doctrine/common, with your branch checked out

[greg0ire]

Aren't there any tests that could be added for this?

[flack]

@greg0ire I don't have time to properly test this right now (will do so later today or tomorrow), but I quickly pulled the branch and ran my unittest suite against it. There were four exceptions thrown (with 3.0.0 the output was clean). There was nothing in the backtrace that pointed explicitly to doctrine/common, so I'll have to investigate a bit further. Pretty sure it's related to this change though (or something else that is in doctrine/common master but not in the 3.0.1 release)

[miloshavlicek]

@greg0ire It seems to be ok in my project.

Here are snippets of generated code from phpunit test:

__CG__DoctrineTestsCommonProxyMagicGetClassWithScalarTypeAndRenamedParameter.php:

    /**
     * {@inheritDoc}
     * @param string $name
     */
    public function __get(string $name): string
    {
        if (\array_key_exists($name, self::$lazyPropertiesNames)) {
            $this->__initializer__ && $this->__initializer__->__invoke($this, '__get', [$name]);
            return $this->$name;
        }

        $this->__initializer__ && $this->__initializer__->__invoke($this, '__get', [$name]);
        return parent::__get($name);
    }

__CG__DoctrineTestsCommonProxyMagicSetClassWithScalarType.php:

    /**
     * {@inheritDoc}
     * @param string $name
     * @param mixed  $value
     */
    public function __set(string $name, $value)
    {
        if (\array_key_exists($name, self::$lazyPropertiesNames)) {
            $this->__initializer__ && $this->__initializer__->__invoke($this, '__set', [$name, $value]);

            $this->$name = $value;

            return;
        }

        $this->__initializer__ && $this->__initializer__->__invoke($this, '__set', [$name, $value]);

        return parent::__set($name, $value);
    }

__CG__DoctrineTestsCommonProxyMagicIssetClassWithInteger.php:

    /**
     * {@inheritDoc}
     * @param  string $name
     * @return boolean
     */
    public function __isset(string $name): int
    {
        if (\array_key_exists($name, self::$lazyPropertiesNames)) {
            $this->__initializer__ && $this->__initializer__->__invoke($this, '__isset', [$name]);

            return isset($this->$name);
        }

        $this->__initializer__ && $this->__initializer__->__invoke($this, '__isset', [$name]);

        return parent::__isset($name);
    }

[miloshavlicek]

@janbarasek Could you please also test it for your project with Nette SmartObject implemented?

[janbarasek]

@miloshavlicek In Nette Framework SmartObject use default parameter names like $name, so it works.

Thanks!

[flack]

@greg0ire did some more digging now and it turns out the exceptions were unrelated, so the fix works on my end. Thx for the quick turnaround everyone!

[miloshavlicek]

@greg0ire @flack Could you please release this patch to 2.13.x branch to solve the issue reported by @flk-a in #869?

[ostrolucky]

We should definitely have a test case for such important fix

[miloshavlicek]

@ostrolucky As I have already mentioned, the test case is here:
__CG__DoctrineTestsCommonProxyMagicGetClassWithScalarTypeAndRenamedParameter.php:

[ostrolucky]

That's not a test case. What I mean is that there should be phpunit assertion which would fail without this patch

[miloshavlicek]

@ostrolucky do you have any suggestion how to test this case? Reflection assertion does not make sense for this bug.

[miloshavlicek]

Is here anyone who can I pay for solving it?

[Ocramius]

Mostly missing a failing test case: could investigate root cause when given an example entity for which an error occurs

[greg0ire]

Can somebody explain to me why this rename is needed? Where does that $field come from in #896 ? Maybe it's obvious to you, but it isn't to me, sorry.

[greg0ire]

Reflection assertion does not make sense for this bug.

Why not? Please explain.

[flack]

@greg0ire not sure if that is what you are asking, but in #896 $field comes from this line in my code:

https://github.com/flack/midgard-portable/blob/master/src/api/dbobject.php#L76

in doctrine/common it comes from here:

https://github.com/doctrine/common/pull/869/files#diff-ce03ab9b396edcbb313c54234c20e0deR538

i.e. since #869 it is read from the class for which the proxy is generated via $methodReflection->getParameters(), before it was just hardcoded to $name

[miloshavlicek]

Can somebody explain to me why this rename is needed? Where does that $field come from in #896 ? Maybe it's obvious to you, but it isn't to me, sorry.

It's not necessary, but it's easier to implement and it's working as expected and does not have any side effects. If someone has a better solution, feel free to send me a PR. My thought was that it's safer because if we would implement any internal variable in the future to methods (&)__get() __set() __isset() it will potentially collide.

[greg0ire]

Is here anyone who can I pay for solving it?

What did you try? What didn't work?

[ostrolucky]

We should rebase this on 2.13.x

[miloshavlicek]

Is here anyone who can I pay for solving it?

What did you try? What didn't it work?

Everything works ok, just nobody approved it to 2.x branch, that's all the problem.

[miloshavlicek]

So test case is added. Any other reviews?

[greg0ire]

Thanks @miloshavlicek !

[greg0ire]

Everything works ok, just nobody approved it to 2.x branch, that's all the problem.

Nobody approved because there were no tests. I wanted to help you write it, but it looks like you managed to do it, great job!