-
-
Notifications
You must be signed in to change notification settings - Fork 294
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
base: 3.2.x
Are you sure you want to change the base?
Don't Generate Set or Unset on Proxy of Readonly Public Properties #957
Conversation
There was a problem hiding this 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:
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 🙂
97b268e
to
830786e
Compare
- Code styling: multi-line control structure - Static analysis: uninitialized readonly property
Thanks for the suggestions! 🙂 |
self::assertStringContainsString( | ||
"'readable' => NULL", | ||
file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyPhp81ReadonlyPublicPropertyType.php') | ||
); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
Better unit testing suggestions by @greg0ire
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. |
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: 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. |
Attempting to fix #954.
getWriteableLazyLoadedPublicPropertiesNames()
__set
or generating calls tounset()
in__construct
and__wakeup
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.