Support readonly properties for read operations

[derrabus]

This PR provides some testcases to check if the ORM handles readonly properties well for read operations. So far, it looks good except for lazy-loaded ManyToOne relations. Here, the proxy tries to unset readonly properties and fails.

[derrabus]

CodeSniffer failure should be fixed by #9317

[beberlei]

The RFC states about Reflection and readonly:

ReflectionProperty::setValue() can bypass the requirement that initialization occurs from the scope where the property has been declared. However, reflection cannot modify a readonly property that has already been initialized.

The problem here I believe is that "simple IDs" are already populated in proxies, and then when loading the entity the ID is set again, triggering this error from UnitOfWork::createEntity that refreshes the proxy.

The workaorund could be for now to avoid using readonly on ID, which makes no sense anyways, because at least for post insert ID generators, it is set to null first, then to an integer when first persisted.

[derrabus]

The problem here I believe is that "simple IDs" are already populated in proxies, and then when loading the entity the ID is set again, triggering this error from UnitOfWork::createEntity that refreshes the proxy.

Why's that? Could we just not set the ID again when resolving the proxy? It should not change anyway, should it?

The workaorund could be for now to avoid using readonly on ID, which makes no sense anyways, because at least for post insert ID generators, it is set to null first, then to an integer when first persisted.

The test assumes that the entity model is used for read operations only. And in that case, readonly IDs are reasonable, I think.

[derrabus]

I've pushed a commit that fixes the issue. Can you have a look?

[beberlei]

This is the hot code path, this change is probably too expensive since it affects every property. I would imagine again we need a special reflection instance here

[derrabus]

Do you already have an idea how to implement that "special reflection instance"? I'm a bit lost here, I'm afraid.

[greg0ire]

@derrabus from what I understood it would look like so:

<?php

declare(strict_types=1);

namespace Doctrine\ORM\Mapping;

use LogicException;
use ReflectionProperty;

use function sprintf;

class ReflectionReadOnlyProperty extends ReflectionProperty
{
    /** @var ReflectionProperty */
    private $originalReflectionProperty;

    public function __construct(ReflectionProperty $originalReflectionProperty)
    {
        $this->originalReflectionProperty = $originalReflectionProperty;
    }

    /**
     * @param object $object
     * @param mixed  $value
     */
    public function setValue($object, $value = null): void
    {
        if ($this->originalReflectionProperty->getValue($object) === $value) {
            return;
        }

        throw new LogicException(sprintf(
            'Attempting to change readonly field %s.',
            $this->originalReflectionProperty->getName()
        ));
    }
}

Then, in ClassMetadataInfo, you would do

                if ($this->reflFields[$field]->isReadOnly()) {
                    $this->reflField[$field] = new ReflectionReadOnlyProperty($this->reflFields[$field])];
                }

where appropriate

[derrabus]

I think I got it right now. 😓

[greg0ire]

Oh yeah 😎

[greg0ire]

Is there a github issue to track this?

[derrabus]

I haven't researched that yet.

[derrabus]

I just filed vimeo/psalm#7357

[greg0ire]

Great! I also observed this locally: https://psalm.dev/r/5c7d43694a , but maybe that's because I'm using 8.0 🤔

[derrabus]

Yes, that error goes away if you run Psalm with PHP 8.1.

[solcik]

@greg0ire @derrabus

Hello, Thank you for your work!

I tried readonly nullable properties in Doctrine entities and I got an error. In Proxy constructor there is unset.

I tried to do failing test case here #9431