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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
629cdb6
Don't Generate Set or Unset on Proxy of Readonly Public Properties
zanbaldwin Feb 11, 2022
830786e
Add Unit Test for Readonly Properties on PHP 8.1
zanbaldwin Feb 11, 2022
d189500
Fix CI Pipeline Errors
zanbaldwin Feb 11, 2022
1521aa8
Test Accessing Readonly Properties after Constructing Proxy
zanbaldwin Feb 12, 2022
306a5ad
Readonly Properties Unit Test: Update Assertions
zanbaldwin Feb 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions lib/Doctrine/Common/Proxy/ProxyGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ public function __construct(?\Closure $initializer = null, ?\Closure $cloner = n

$toUnset = array_map(static function (string $name): string {
return '$this->' . $name;
}, $this->getLazyLoadedPublicPropertiesNames($class));
}, $this->getWriteableLazyLoadedPublicPropertiesNames($class));

return $constructorImpl . ($toUnset === [] ? '' : ' unset(' . implode(', ', $toUnset) . ");\n")
. <<<'EOT'
Expand Down Expand Up @@ -591,7 +591,7 @@ public function {$returnReference}__get($parametersString)$returnTypeHint
*/
private function generateMagicSet(ClassMetadata $class)
{
$lazyPublicProperties = $this->getLazyLoadedPublicPropertiesNames($class);
$lazyPublicProperties = $this->getWriteableLazyLoadedPublicPropertiesNames($class);
$reflectionClass = $class->getReflectionClass();
$hasParentSet = false;
$inheritDoc = '';
Expand Down Expand Up @@ -808,7 +808,7 @@ private function generateWakeupImpl(ClassMetadata $class)
$hasParentWakeup = $reflectionClass->hasMethod('__wakeup');

$unsetPublicProperties = [];
foreach ($this->getLazyLoadedPublicPropertiesNames($class) as $lazyPublicProperty) {
foreach ($this->getWriteableLazyLoadedPublicPropertiesNames($class) as $lazyPublicProperty) {
$unsetPublicProperties[] = '$this->' . $lazyPublicProperty;
}

Expand Down Expand Up @@ -1005,6 +1005,32 @@ private function isShortIdentifierGetter($method, ClassMetadata $class)
return false;
}

/**
* Generates the list of public properties to be lazy loaded, that are writable.
*
* @return list<string>
*/
public function getWriteableLazyLoadedPublicPropertiesNames(ClassMetadata $class): array
{
$properties = [];

foreach ($class->getReflectionClass()->getProperties(ReflectionProperty::IS_PUBLIC) as $property) {
$name = $property->getName();

if (
(! $class->hasField($name) && ! $class->hasAssociation($name))
|| $class->isIdentifier($name)
|| (method_exists($property, 'isReadOnly') && $property->isReadOnly())
) {
continue;
}

$properties[] = $name;
}

return $properties;
}

/**
* Generates the list of public properties to be lazy loaded.
*
Expand Down
16 changes: 16 additions & 0 deletions tests/Doctrine/Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

namespace Doctrine\Tests\Common\Proxy;

class Php81ReadonlyPublicPropertyType
{
public readonly string $readable;
public string $writeable = 'default';

public function __construct(
public readonly string $id,
string $readable = 'readable-default'
) {
$this->readable = $readable;
}
}
35 changes: 35 additions & 0 deletions tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,41 @@ public function testPhp81NeverType()
);
}

/**
* @requires PHP >= 8.1.0
*/
public function testPhp81ReadonlyPublicProperties()
{
$className = Php81ReadonlyPublicPropertyType::class;
$proxyClassName = 'Doctrine\Tests\Common\ProxyProxy\__CG__\Php81ReadonlyPublicPropertyType';

if ( ! class_exists($proxyClassName, false)) {
$metadata = $this->createClassMetadata($className, ['id']);

$metadata
->expects($this->any())
greg0ire marked this conversation as resolved.
Show resolved Hide resolved
->method('hasField')
->will($this->returnCallback(static function ($fieldName) {
return in_array($fieldName, ['id', 'readable', 'writeable']);
}));

$proxyGenerator = new ProxyGenerator(__DIR__ . '/generated', __NAMESPACE__ . 'Proxy');
$this->generateAndRequire($proxyGenerator, $metadata);
}

// Readonly properties are removed from unset.
self::assertStringContainsString(
'unset($this->writeable);',
file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyPhp81ReadonlyPublicPropertyType.php')
);

// But remain in property listings.
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 🤞

Copy link

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.

}

/**
* @requires PHP >= 8.1.0
*/
Expand Down