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

[zanbaldwin]

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.

[greg0ire]

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:

common/tests/Doctrine/Tests/Common/Proxy/ProxyMagicMethodsTest.php

Lines 80 to 106 in 295082d

	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 🙂

[zanbaldwin]

Thanks for the suggestions! 🙂

[greg0ire]

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

[zanbaldwin]

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.

[greg0ire]

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

[zanbaldwin]

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.

[greg0ire]

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

[zanbaldwin]

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.

[zanbaldwin]

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

[greg0ire]

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

[chapterjason]

@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]

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]

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.