doctrine/archived-orm
1
0
Fork 0
mirror of https://github.com/doctrine/orm.git synced 2026-03-24 06:52:09 +01:00
Code Issues 5.3k Packages Projects Releases 100 Wiki Activity

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

New Issue
Open
opened 2026-01-22 15:41:35 +01:00 by admin · 13 comments
admin commented 2026-01-22 15:41:35 +01:00
Owner
Copy Link

Originally created by @zanbaldwin on GitHub (Feb 12, 2022).

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.

Originally created by @zanbaldwin on GitHub (Feb 12, 2022). ### Bug Report <!-- Fill in the relevant information below to help triage your issue. --> | 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). ```php #[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, ) {} } ``` ```php $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`. ```diff 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.
admin added the Bug label 2026-01-22 15:41:35 +01:00
admin commented 2026-01-22 15:41:36 +01:00
Author
Owner
Copy Link

@derrabus commented on GitHub (Feb 15, 2022):

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.

@derrabus commented on GitHub (Feb 15, 2022): 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.
admin commented 2026-01-22 15:41:36 +01:00
Author
Owner
Copy Link

@zanbaldwin commented on GitHub (Feb 15, 2022):

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.

@zanbaldwin commented on GitHub (Feb 15, 2022): 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 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.
admin commented 2026-01-22 15:41:36 +01:00
Author
Owner
Copy Link

@garak commented on GitHub (Feb 16, 2022):

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

@garak commented on GitHub (Feb 16, 2022): What about doing a non-strict comparison for objects? I.e., using `==` instead of `===`.
admin commented 2026-01-22 15:41:36 +01:00
Author
Owner
Copy Link

@derrabus commented on GitHub (Feb 16, 2022):

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?

@derrabus commented on GitHub (Feb 16, 2022): 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?
admin commented 2026-01-22 15:41:37 +01:00
Author
Owner
Copy Link

@garak commented on GitHub (Feb 16, 2022):

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.

@garak commented on GitHub (Feb 16, 2022): What's the problem with non-strict comparison? According to the [manual](https://www.php.net/manual/en/language.oop5.object-comparison.php), it should work comparing objects values, and I think it can fit both the cases of DateTime and of unique identifiers.
admin commented 2026-01-22 15:41:38 +01:00
Author
Owner
Copy Link

@zanbaldwin commented on GitHub (Feb 16, 2022):

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
@zanbaldwin commented on GitHub (Feb 16, 2022): 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: ```php $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: ```php 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 ```
admin commented 2026-01-22 15:41:38 +01:00
Author
Owner
Copy Link

@RSickenberg commented on GitHub (Mar 21, 2022):

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

@RSickenberg commented on GitHub (Mar 21, 2022): Same issue with `$doctrine->remove($object)` with a `readonly $id`
admin commented 2026-01-22 15:41:38 +01:00
Author
Owner
Copy Link

@nesl247 commented on GitHub (Jun 29, 2022):

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

@nesl247 commented on GitHub (Jun 29, 2022): @zanbaldwin your patch fixed it for me. I do hope there can be some official progress on this but until then, thanks!
admin commented 2026-01-22 15:41:38 +01:00
Author
Owner
Copy Link

@nCrazed commented on GitHub (Aug 18, 2022):

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

@nCrazed commented on GitHub (Aug 18, 2022): Would exposing a configuration hook for providing custom equality checks be acceptable here?
admin commented 2026-01-22 15:41:39 +01:00
Author
Owner
Copy Link

@Invis1ble commented on GitHub (Aug 8, 2024):

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

@Invis1ble commented on GitHub (Aug 8, 2024): This needs to be fixed as soon as possible because, at the moment, we can't use readonly Value Objects as properties.
admin commented 2026-01-22 15:41:40 +01:00
Author
Owner
Copy Link

@bigfoot90 commented on GitHub (Oct 24, 2024):

Any news on this? ♥

@bigfoot90 commented on GitHub (Oct 24, 2024): Any news on this? ♥
admin commented 2026-01-22 15:41:40 +01:00
Author
Owner
Copy Link

@Arkemlar commented on GitHub (Oct 24, 2025):

The problem easaly arises when you try to lock something. Internally it leads to refresh() that attempts to update readonly properties like uuid, createdAt, and so on.
Example:

Image Image

See! Both values are the same. But doctrine tries to set and fails. This forces developers to not use readonly for mapped properties, that is kind of code-uglifying IMHO.

@Arkemlar commented on GitHub (Oct 24, 2025): The problem easaly arises when you try to lock something. Internally it leads to refresh() that attempts to update readonly properties like `uuid`, `createdAt`, and so on. Example: <img width="999" height="118" alt="Image" src="https://github.com/user-attachments/assets/7b77b005-2aab-415e-9607-30488f1fdc57" /> <img width="458" height="383" alt="Image" src="https://github.com/user-attachments/assets/7431223f-537d-47e8-a1f2-bd247023ece8" /> See! Both values are the same. But doctrine tries to set and fails. This forces developers to not use readonly for mapped properties, that is kind of code-uglifying IMHO.
admin commented 2026-01-22 15:41:40 +01:00
Author
Owner
Copy Link

@Arkemlar commented on GitHub (Nov 2, 2025):

@beberlei @greg0ire @derrabus I kindly ask you to react on this.

I suppose readonly property should be handled by doctrine correctly: once set - either by first hydration OR after the object has been created in userland code - it SHOULD NOT be changed ever. No matter either it value object, scalar or whatever else.

At this moment doctrine punches with LogicException and text like Attempting to change readonly property App\...\SaleOrderLine::$uuid. This requires to sadly remove readonly for mapped properties of every entity that might be refreshed or used in find($id, LockMode::PESSIMISTIC_*). In latter case pessimistic locking on already managed entity leads to refreshing it (and refresh failing on readonly property).
The problem resides in this line https://github.com/doctrine/orm/blob/3.5.x/src/UnitOfWork.php#L2420

Are you agree with this change?

@Arkemlar commented on GitHub (Nov 2, 2025): @beberlei @greg0ire @derrabus I kindly ask you to react on this. I suppose readonly property should be handled by doctrine correctly: once set - either by first hydration OR after the object has been created in userland code - it SHOULD NOT be changed ever. No matter either it value object, scalar or whatever else. At this moment doctrine punches with LogicException and text like `Attempting to change readonly property App\...\SaleOrderLine::$uuid`. This requires to sadly remove `readonly` for mapped properties of every entity that might be refreshed or used in `find($id, LockMode::PESSIMISTIC_*)`. In latter case pessimistic locking on already managed entity leads to refreshing it (and refresh failing on readonly property). The problem resides in this line https://github.com/doctrine/orm/blob/3.5.x/src/UnitOfWork.php#L2420 Are you agree with this change?
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
BC Break
Bug
Can't Fix
Deprecation
Documentation
DQL
Duplicate
Failing Test
Hacktoberfest
Improvement
Improvement
Incomplete
Invalid
Missing Tests
New Feature
PR Created
pull-request
Mirrored from GitHub Pull Request
 Question
Regression
Status: Waiting feedback
Waiting feedback
WIP
Won't Fix
No Label Bug
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: doctrine/archived-orm#6930
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?