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

Don't Generate Set or Unset on Proxy of Readonly Public Properties #957

Open
wants to merge 5 commits into
base: 3.2.x
Choose a base branch
from

Conversation

zanbaldwin
Copy link

@zanbaldwin zanbaldwin commented Feb 11, 2022

Attempting to fix #954.

  • Create another private method getWriteableLazyLoadedPublicPropertiesNames()
  • Use this method when generating __set or generating calls to unset() in __construct and __wakeup
  • Leave all other methods that referenced getLazyLoadedPublicPropertiesNames() as they were.

Would like input on these changes as I don't have enough knowledge to know if omitting these properties will have unintended side-effects elsewhere.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that for the test, you would need to create a class with a readonly property, then generate a proxy for it, then instantiate that proxy.

Here is an existing test that seems to do that:

public function testInheritedMagicGetWithScalarType()
{
$proxyClassName = $this->generateProxyClass(MagicGetClassWithScalarType::class);
$proxy = new $proxyClassName(
static function (Proxy $proxy, $method, $params) use (&$counter) {
if (! in_array($params[0], ['publicField', 'test', 'notDefined'])) {
throw new InvalidArgumentException('Unexpected access to field "' . $params[0] . '"');
}
$initializer = $proxy->__getInitializer();
$proxy->__setInitializer(null);
$proxy->publicField = 'modifiedPublicField';
$counter += 1;
$proxy->__setInitializer($initializer);
}
);
self::assertSame('id', $proxy->id);
self::assertSame('modifiedPublicField', $proxy->publicField);
self::assertSame('test', $proxy->test);
self::assertSame('not defined', $proxy->notDefined);
self::assertSame(3, $counter);
}

Once you have that, you can validate that the bug no longer happens 🙂

lib/Doctrine/Common/Proxy/ProxyGenerator.php Outdated Show resolved Hide resolved
@greg0ire greg0ire added the Bug label Feb 11, 2022
@zanbaldwin zanbaldwin force-pushed the bug-unset-readonly-properties-in-proxy-constructor branch from 97b268e to 830786e Compare February 11, 2022 19:37
- Code styling: multi-line control structure
- Static analysis: uninitialized readonly property
@zanbaldwin
Copy link
Author

Thanks for the suggestions! 🙂

self::assertStringContainsString(
"'readable' => NULL",
file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyPhp81ReadonlyPublicPropertyType.php')
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be great to do a new $proxyClassName here and perform more assertions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm at the point where I don't know how to proceed, I've tried setting data inside the proxy initialization proxy using the following:

$initializationData = [
    'id' => 'c0b5cb93-f01b-43f8-bc66-bc943b1ebcfd',
    'readable' => 'This field is read-only.',
    'writeable' => 'This field is writeable.',
];
$proxy = new $proxyClassName(static function (Proxy $proxy, $method, $params) use (&$counter, $initializationData) {
    if (!in_array($params[0], array_keys($initializationData))) {
        throw new InvalidArgumentException(
            sprintf('Should not be initialized when checking isset("%s")', $params[0])
        );
    }
    $initializer = $proxy->__getInitializer();
    $proxy->__setInitializer(null);
    $proxy->{$params[0]} = $initializationData[$params[0]];
    $counter += 1;
    $proxy->__setInitializer($initializer);
});
self::assertTrue(isset($proxy->id));

Not sure how to initialize a readonly property from the proxy initializer closure, when any readonly property initialization is required to happen from inside the class itself.

There must be a way since ORM supports readonly properties, but I've reached my limit for tonight as I found another potential bug while diving into the UnitOfWork internals.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can define another closure inside the first one, but this time, bind it to the proxy? Like this: https://3v4l.org/Q1rTq#v8.1.2

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a good idea, but the initializer closure is never getting called because PHP core is throwing an error (trying to access uninitialized property) before any magic methods are called. I'm officially stumped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's confusing indeed! Do you have a stack trace with that error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A print_r of the error thrown by the above code, which isn't very helpful - it looks like an error is thrown as soon as it gets to trying to access the property and doesn't proceed any further:

Error Object
(
    [message:protected] => Typed property Doctrine\Tests\Common\Proxy\Php81ReadonlyPublicPropertyType::$id must not be accessed before initialization
    [string:Error:private] =>
    [code:protected] => 0
    [file:protected] => /doctrine-common/tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php
    [line:protected] => 575
    [trace:Error:private] => (replaced with Throwable::getTraceAsString)
        #0 /doctrine-common/vendor/phpunit/phpunit/src/Framework/TestCase.php(1545): Doctrine\Tests\Common\Proxy\ProxyGeneratorTest->testPhp81ReadonlyPublicProperties()
        #1 /doctrine-common/vendor/phpunit/phpunit/src/Framework/TestCase.php(1151): PHPUnit\Framework\TestCase->runTest()
        #2 /doctrine-common/vendor/phpunit/phpunit/src/Framework/TestResult.php(726): PHPUnit\Framework\TestCase->runBare()
        #3 /doctrine-common/vendor/phpunit/phpunit/src/Framework/TestCase.php(903): PHPUnit\Framework\TestResult->run()
        #4 /doctrine-common/vendor/phpunit/phpunit/src/Framework/TestSuite.php(677): PHPUnit\Framework\TestCase->run()
        #5 /doctrine-common/vendor/phpunit/phpunit/src/Framework/TestSuite.php(677): PHPUnit\Framework\TestSuite->run()
        #6 /doctrine-common/vendor/phpunit/phpunit/src/Framework/TestSuite.php(677): PHPUnit\Framework\TestSuite->run()
        #7 /doctrine-common/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(670): PHPUnit\Framework\TestSuite->run()
        #8 /doctrine-common/vendor/phpunit/phpunit/src/TextUI/Command.php(143): PHPUnit\TextUI\TestRunner->run()
        #9 /doctrine-common/vendor/phpunit/phpunit/src/TextUI/Command.php(96): PHPUnit\TextUI\Command->run()
        #10 /doctrine-common/vendor/phpunit/phpunit/phpunit(98): PHPUnit\TextUI\Command::main()
        #11 /doctrine-common/vendor/bin/phpunit(110): include('...')
        #12 {main}
    [previous:Error:private] =>
)

I would ask if my proxy initializer closure is correct, but I don't think it's even getting to that point:

$proxy = new $proxyClassName(static function (Proxy $proxy, $method, $params) {
    echo 'Proxy initialization closure called for property "' . $params[0] . '".';
    exit;
});

var_dump(isset($proxy->id)); // will always return false.
var_dump($proxy->id)         // throws Error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May have to pass in initial values for the readonly properties into the proxy constructor, which kinda defeats the whole point of a proxy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beberlei @derrabus @malarzm might have some ideas here 🤞

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greg0ire @zanbaldwin I had an issue with the same topic:

doctrine/orm#10049
doctrine/orm#10059

You can set properties by changing the scope of the reflection property.

@beberlei
Copy link
Member

This has the unintended consequence of breaking lazy loading if the user acceses the readonly public property of an not yet loaded proxy, then it will not be loaded leaving the property NULL.

@alessandromorelli
Copy link

The readonly public properties should be handled separately from the other public properties.

The readonly public properties should be unset but should not be initialized with a default, to let the initializer free to write them.

I cobbled up a proof of concept:
alessandromorelli@7f9d1d0

Readonly properties may be unset only in the same scope in which they are declared, so I've bound a Closure to the proxy's parent class.

Once unset, any read access to the property triggers the __get method and the initializer can insert the values.

If the initializer does not actually initialize the readonly property, any write access triggers a "Cannot initialize readonly property XXXX from scope YYYY", which is a bit confusing but shouldn't normally happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Public readonly properties are unset in Proxy's constructor
5 participants