Bug: UnitOfWork attempts to update readonly property (object IDs don't match)

[zanbaldwin]

Bug Report

Q	A
BC Break	no
ORM Version	2.11.x
PHP Version	8.1+

Summary

Found a bug where UnitOfWork attempts to update a readonly property of a type that is an object not managed by Doctrine (eg, value object). a \LogicException is thrown.

Current behavior

Even though the data has not changed, the value object inside the managed entity (held inside UnitOfWork::$identityMap) is a different instance to the value object constructed from the data/row fetched from the database. When ReflectionReadonlyProperty::setValue() does an identical comparison === the object IDs don't match and a LogicException is thrown.

How to reproduce

Set an entity column property to be both readonly and type-hinted to be an object that is not an entity managed by the object manager (eg, Symfony\Component\Uid\Ulid, App\ValueObject\Email, \DateTimeImmutable, etc).

#[ORM\Entity]
class Test1
{
    #[ORM\Id]
    #[ORM\Column(name: 'id', type: 'integer', nullable: false)]
    #[ORM\GeneratedValue(strategy: 'AUTO')]
    public readonly int $id;

    public function __construct(
        #[ORM\Column(name: 'date', type: 'datetime', nullable: false)]
        public readonly \DateTimeImmutable $date = new \DateTime,
    ) {}
}

$entityManager->persist($entity = new TestEntity);
$entityManager->flush();
$entityManager->refresh($entity);

Expected behavior

Read-only properties should never be written to after initialization.

However, it's logical to assume that someone is calling ObjectManager::refresh() because they expect the data to have changed in the database. In this case, the only way I can think to fetch that updated data would be to detach the managed entity first before loading again.

Workaround

I've temporarily avoided this issue by not resetting readonly properties that have already been initialized, but it's not a solution since it purposefully avoids performing that value comparison inside ReflectionReadonlyProperty.

index cd5d72716..aadc82d32 100644
--- a/lib/Doctrine/ORM/UnitOfWork.php
+++ b/lib/Doctrine/ORM/UnitOfWork.php
@@ -2740,7 +2740,11 @@ class UnitOfWork implements PropertyChangedListener
         }
 
         foreach ($data as $field => $value) {
-            if (isset($class->fieldMappings[$field])) {
+            if (isset($class->fieldMappings[$field]) && (
+                !method_exists($class->reflFields[$field], 'isReadOnly')
+                || !$class->reflFields[$field]->isReadOnly()
+                || !$class->reflFields[$field]->isInitialized($entity)
+            )) {
                 $class->reflFields[$field]->setValue($entity, $value);
             }
         }

I also thought about using the updatable value from the column field mapping data, or using $this->fieldMappings[$field]['notUpdatable'], but I'm unsure how this would affect setting the value of generated columns.

[derrabus]

This is kind of tricky because we suddenly need a way to tell if two arbitrary objects are equal. We can add a special case for datetime objects, but as you already mentioned, this will again fail for custom userland types.

[zanbaldwin]

Off the top of my head the only way I can think of doing this in PHP without building some kind of object comparison library (which seems overkill) is something along the lines of using serialize.

I haven't checked the performance of this and I'm sure there'll be a whole host of problems I haven't foreseen, but what's your opinion on something like this? Once execution has got to this point we know we're dealing with a read-only, initialized property that's a value object.

diff --git i/lib/Doctrine/ORM/Mapping/ReflectionReadonlyProperty.php w/lib/Doctrine/ORM/Mapping/ReflectionReadonlyProperty.php
index fba314be1..7cfe2b545 100644
--- i/lib/Doctrine/ORM/Mapping/ReflectionReadonlyProperty.php
+++ w/lib/Doctrine/ORM/Mapping/ReflectionReadonlyProperty.php
@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace Doctrine\ORM\Mapping;
 
+use Exception;
 use InvalidArgumentException;
 use LogicException;
 use ReflectionProperty;
@@ -12,6 +13,7 @@ use function assert;
 use function func_get_args;
 use function func_num_args;
 use function is_object;
+use function serialize;
 use function sprintf;
 
 /**
@@ -45,6 +47,13 @@ final class ReflectionReadonlyProperty extends ReflectionProperty
         assert(is_object($objectOrValue));
 
         if (parent::getValue($objectOrValue) !== $value) {
+            try {
+                if (is_object($value) && serialize(parent::getValue($objectOrValue)) === serialize($value)) {
+                    return;
+                }
+            } catch (Exception $e) {
+            }
+
             throw new LogicException(sprintf('Attempting to change readonly property %s::$%s.', $this->class, $this->name));
         }
     }

I can't imagine a use-case where a value object has been constructed from a Doctrine Type, and would result in serialized data that's different to the actual property.

[garak]

What about doing a non-strict comparison for objects? I.e., using == instead of ===.

[derrabus]

Depending on the actual custom types, both approaches might or might not work. But they boil down to the UoW having an opionion on whether two arbitrary objects are equal or not. I don't think it's a good idea having that kind of guesswork in the ORM.

Maybe we should just skip initialized readonly fields when refreshing an entity after insert and document this as a known limitation?

[garak]

What's the problem with non-strict comparison? According to the manual, it should work comparing objects values, and I think it can fit both the cases of DateTime and of unique identifiers.

[zanbaldwin]

I've been playing around with both methods, and I'm beginning to agree with @derrabus - perhaps we should just skip initialized readonly fields?

We don't know how complex the object might be (for example, in the past I've created a custom Doctrine Type to construct Opis\JsonSchema\Schema objects from a json column). Problems with comparison arise whenever the objects being compared contain either a DateTime (those pesky milliseconds) or a closure, so while they may be functionally the same we just won't be able to detect it:

$mysqlFormatString = 'Y-m-d H:i:s';
// Constructed by userland-code.
$userlandDate = new DateTime;
// Constructed by DateTimeType on MySQL platform.
$databaseDate = DateTime::createFromFormat($mysqlFormatString, $userlandDate->format($mysqlFormatString));

var_dump($userlandDate == $databaseDate); // bool(false)
var_dump(serialize($userlandDate) === serialize($databaseDate)); // bool(false)

While we could make a special case for DateTimeInterface objects, we wouldn't be able to do anything about them if they're nested deep in the objects properties. And closures are a big no-go:

class ContainsClosure {
    private \Closure $closure;
    public function __construct() {
        $this->closure = function (string $bar) {
            return $bar;
        };
    }
}
var_dump(new ContainsClosure == new ContainsClosure); // bool(false)
var_dump(serialize(new ContainsClosure) === serialize(new ContainsClosure)); // throws \Exception

[RSickenberg]

Same issue with $doctrine->remove($object) with a readonly $id

[nesl247]

@zanbaldwin your patch fixed it for me. I do hope there can be some official progress on this but until then, thanks!

[nCrazed]

Would exposing a configuration hook for providing custom equality checks be acceptable here?

[Invis1ble]

This needs to be fixed as soon as possible because, at the moment, we can't use readonly Value Objects as properties.