Allow to not Compare Object Types by reference #4931

Open
opened 2026-01-22 14:52:18 +01:00 by admin · 43 comments
Owner

Originally created by @beberlei on GitHub (Dec 10, 2015).

If we have a Doctrine DBAL type with an object (DateTime, any custom object), then its compared by reference and we cannot use mutable objects.

We should consider allowing a way to have mutable objects here and delegate the changeset compuutation to a service or do something clever.

Originally created by @beberlei on GitHub (Dec 10, 2015). If we have a Doctrine DBAL type with an object (DateTime, any custom object), then its compared by reference and we cannot use mutable objects. We should consider allowing a way to have mutable objects here and delegate the changeset compuutation to a service or do something clever.
Author
Owner

@Ocramius commented on GitHub (Dec 10, 2015):

I proposed comparing the DB-side representation in the past, but it may be
expensive...
On Dec 10, 2015 11:23, "Benjamin Eberlei" notifications@github.com wrote:

If we have a Doctrine DBAL type with an object (DateTime, any custom
object), then its compared by reference and we cannot use mutable objects.

We should consider allowing a way to have mutable objects here and
delegate the changeset compuutation to a service or do something clever.


Reply to this email directly or view it on GitHub
https://github.com/doctrine/doctrine2/issues/5542.

@Ocramius commented on GitHub (Dec 10, 2015): I proposed comparing the DB-side representation in the past, but it may be expensive... On Dec 10, 2015 11:23, "Benjamin Eberlei" notifications@github.com wrote: > If we have a Doctrine DBAL type with an object (DateTime, any custom > object), then its compared by reference and we cannot use mutable objects. > > We should consider allowing a way to have mutable objects here and > delegate the changeset compuutation to a service or do something clever. > > — > Reply to this email directly or view it on GitHub > https://github.com/doctrine/doctrine2/issues/5542.
Author
Owner

@beberlei commented on GitHub (Dec 10, 2015):

@Ocramius maybe we can enable this comparison based on a setting, so its only done for fields where its necessary.

@beberlei commented on GitHub (Dec 10, 2015): @Ocramius maybe we can enable this comparison based on a setting, so its only done for fields where its necessary.
Author
Owner

@DHager commented on GitHub (Dec 10, 2015):

I expect that if someone has something like a DateTime or ColorCode, they will always want it to be compared via the same mechanism, regardless of which entity it appears on. So then it's the type of the field which implicitly determines the comparison-rules.

How about making it so that an EntityManager always has an, uhm, EntityScalarComparator entities, which it asks for answers about "did this change" query, even when both inputs are strings. An additional PHP method-call in the stack is a small price to pay for separating out the complexity.

Then EntityScalarComparator is where you might , say, have an option to change how DateTime is compared, or a mechanism to chain/overlay another comparison service to support your own custom objects.

@DHager commented on GitHub (Dec 10, 2015): I expect that if someone has something like a `DateTime` or `ColorCode`, they will _always_ want it to be compared via the same mechanism, regardless of which entity it appears on. So then it's the type of the field which implicitly determines the comparison-rules. How about making it so that an `EntityManager` always has an, uhm, `EntityScalarComparator` entities, which it asks for answers about "did this change" query, even when both inputs are strings. An additional PHP method-call in the stack is a small price to pay for separating out the complexity. Then `EntityScalarComparator` is where you might , say, have an option to change how `DateTime` is compared, or a mechanism to chain/overlay another comparison service to support your own custom objects.
Author
Owner

@Ocramius commented on GitHub (Dec 10, 2015):

@Ocramius maybe we can enable this comparison based on a setting, so its only done for fields where its necessary.

Possibly, but I'd still look at it after @guilhermeblanco is done with digging through JPA's field mappings.

Then EntityScalarComparator is where you might , say, have an option to change how DateTime is compared, or a mechanism to chain/overlay another comparison service to support your own custom objects.

The idea is to generate specific comparators per entity (you pretty much always compare entity to entity, not just single fields).

@Ocramius commented on GitHub (Dec 10, 2015): > @Ocramius maybe we can enable this comparison based on a setting, so its only done for fields where its necessary. Possibly, but I'd still look at it after @guilhermeblanco is done with digging through JPA's field mappings. > Then `EntityScalarComparator` is where you might , say, have an option to change how `DateTime` is compared, or a mechanism to chain/overlay another comparison service to support your own custom objects. The idea is to generate specific comparators per entity (you pretty much always compare entity to entity, not just single fields).
Author
Owner

@DHager commented on GitHub (Dec 11, 2015):

The idea is to generate specific comparators per entity (you pretty much always compare entity to entity, not just single fields).

I interpreted @beberlei as describing a different scenario, where there is only one Doctrine Entity, and someone has used a custom DBAL mapping type for a specific field on it. Then the issue is with how the ORM does dirty-checking on each of the fields.

For example, suppose someone creates a BitMask DBAL type that saves/loads from an SQL integer column. There are two possible failure-cases:

  1. If the BitMask instance is mutable, then the ORM might fail to commit changes, because as far as it is concerned the field-object always === itself. That's why we "cannot use mutable objects".
  2. If the BitMask (mutable or otherwise) is replaced by another instance which happens to have identical content, then the ORM will consider the Entity dirty, even if $mask1->equals($mask2)

The underlying behavior is like:

if($oldVal !== $curVal){
    // Yes, add to computed changeset
}
// No, continue comparing other properties

But for that particular class of field-representing-object, the designer might want custom behavior more like:

if($oldVal !== $curVal){
    if($helper->areTheyEquivalent($oldVal,$curVal)){
        // Yes, add to computed changeset
    }      
}else{
    if($helper->isItMutableAndDirty($oldVal)){
        // Yes, add to computed changeset
    }
}
// No, continue comparing other properties
@DHager commented on GitHub (Dec 11, 2015): > The idea is to generate specific comparators per entity (you pretty much always compare entity to entity, not just single fields). I interpreted @beberlei as describing a different scenario, where there is only one Doctrine Entity, and someone has used a [custom DBAL mapping type](http://doctrine-orm.readthedocs.org/projects/doctrine-dbal/en/latest/reference/types.html#custom-mapping-types) for a specific field on it. Then the issue is with [how the ORM does dirty-checking](https://github.com/doctrine/doctrine2/blob/bd949312016be418c36603d89951b9641f79ffdb/lib/Doctrine/ORM/UnitOfWork.php#L660) on each of the fields. For example, suppose someone creates a `BitMask` DBAL type that saves/loads from an SQL integer column. There are two possible failure-cases: 1. If the `BitMask` instance is mutable, then the ORM might fail to commit changes, because as far as it is concerned the field-object always `===` itself. That's why we "cannot use mutable objects". 2. If the `BitMask` (mutable or otherwise) is replaced by another instance which happens to have _identical content_, then the ORM will consider the Entity dirty, even if `$mask1->equals($mask2)` The underlying behavior is like: ``` if($oldVal !== $curVal){ // Yes, add to computed changeset } // No, continue comparing other properties ``` But for that particular class of field-representing-object, the designer might want custom behavior more like: ``` if($oldVal !== $curVal){ if($helper->areTheyEquivalent($oldVal,$curVal)){ // Yes, add to computed changeset } }else{ if($helper->isItMutableAndDirty($oldVal)){ // Yes, add to computed changeset } } // No, continue comparing other properties ```
Author
Owner

@Steveb-p commented on GitHub (Apr 5, 2016):

Probably affects objects serialization: http://stackoverflow.com/questions/30193351/how-to-update-doctrine-object-type-field

@Steveb-p commented on GitHub (Apr 5, 2016): Probably affects objects serialization: http://stackoverflow.com/questions/30193351/how-to-update-doctrine-object-type-field
Author
Owner

@Fedik commented on GitHub (Nov 7, 2017):

I have a couple custom types, and it a bit annoying 😄 , so I got 2 ideas:

  1. Since all mapping types comes from Doctrine DBAL, then compassion should be done also by Doctrine\DBAL\Types\Type.

Maybe can just add one method to the Doctrine\DBAL\Types\Type, which by default use strict === compassion, and for custom Type can be overridden.

// Default
public function compareValues($old, $new){
  return $old === $new;
}

// For DateTime
public function compareValues($old, $new){
  return (string) $old === (string) $new;
}

then UOW just do $type->compareValues($old, $new);

Is it expensive?

  1. Another way, check whether we can cast object to string, and compare strings:
$isDifferent = is_object($old) && method_exists($old, '__toString') 
  ? (string) $old === (string) $new
  : $old === $new;
// Of course need to check that both $new and $old is not empty

Which is less expensive?

The first one, is more flexible, and gives some freedom for developers, but require changes in both DBAL and ORM. Second one is more simple (can be done only in ORM side), but it also less flexible.

@Fedik commented on GitHub (Nov 7, 2017): I have a couple custom types, and it a bit annoying 😄 , so I got 2 ideas: 1) Since all mapping types comes from Doctrine DBAL, then compassion should be done also by `Doctrine\DBAL\Types\Type`. Maybe can just add one method to the `Doctrine\DBAL\Types\Type`, which by default use strict `===` compassion, and for custom Type can be overridden. ```php // Default public function compareValues($old, $new){ return $old === $new; } // For DateTime public function compareValues($old, $new){ return (string) $old === (string) $new; } ``` then UOW just do `$type->compareValues($old, $new);` Is it expensive? 2) Another way, check whether we can cast object to string, and compare strings: ```php $isDifferent = is_object($old) && method_exists($old, '__toString') ? (string) $old === (string) $new : $old === $new; // Of course need to check that both $new and $old is not empty ``` Which is less expensive? The first one, is more flexible, and gives some freedom for developers, but require changes in both DBAL and ORM. Second one is more simple (can be done only in ORM side), but it also less flexible.
Author
Owner

@Ocramius commented on GitHub (Nov 7, 2017):

Both are extremely expensive, since an additional set of method calls per field per value per stored UoW change is to be performed. This is going to massively affect ORM performance, and can only be done if the checks can be compiled into the UoW.

@Ocramius commented on GitHub (Nov 7, 2017): Both are extremely expensive, since an additional set of method calls per field per value per stored UoW change is to be performed. This is going to massively affect ORM performance, and can only be done if the checks can be compiled into the UoW.
Author
Owner

@Fedik commented on GitHub (Nov 7, 2017):

okay, more complicated than I thought

can only be done if the checks can be compiled into the UoW

what it means? sorry, not very understood
hm, okay I think I understood

(string) $old === (string) $new

yeah that can be more expansive, because __toString can contain extra complex logic,
then Doctrine\DBAL\Types\Type looks more simple betwen these both, but performance still depend from what developer put inside Doctrine\DBAL\Types\MyCustomType::compareValues().

hm, okay, not perfect

@Fedik commented on GitHub (Nov 7, 2017): okay, more complicated than I thought > can only be done if the checks can be compiled into the UoW ~what it means? sorry, not very understood~ hm, okay I think I understood > (string) $old === (string) $new yeah that can be more expansive, because `__toString` can contain extra complex logic, then `Doctrine\DBAL\Types\Type` looks more simple betwen these both, but performance still depend from what developer put inside `Doctrine\DBAL\Types\MyCustomType::compareValues()`. hm, okay, not perfect
Author
Owner

@Fedik commented on GitHub (Nov 8, 2017):

I have made tests.
The full source of the test is there https://github.com/Fedik/doctrine-changeset-test
(all time in seconds)

default

Dummies 10000 items
Make dummies: 3.6029109954834
Compute changes: 0.1790030002594
Recompute changes: 0.22904109954834

method_exists($orgValue, '__toString')

Dummies 10000 items
Make dummies: 3.6226229667664
Compute changes: 0.18751001358032
Recompute changes: 0.23972296714783

note: DateTime does not have __toString so it ignored here

Doctrine\DBAL\Types\Type::isValuesIdentical($val1, $val2)

Dummies 10000 items
Make dummies: 3.6306829452515
Compute changes: 0.22055912017822
Recompute changes: 0.24937987327576

The simple objects like Point do not make huge difference, it around the same with default.
But more complex Objects can take some more time, that true.

From my point of view Doctrine\DBAL\Types\Type::isValuesIdentical($val1, $val2) is good enough to accept (but still not perfect 😉 )

if I have missed something in my test, please tell me.

@Fedik commented on GitHub (Nov 8, 2017): I have made tests. The full source of the test is there https://github.com/Fedik/doctrine-changeset-test _(all time in seconds)_ #### default ``` Dummies 10000 items Make dummies: 3.6029109954834 Compute changes: 0.1790030002594 Recompute changes: 0.22904109954834 ``` #### ``method_exists($orgValue, '__toString')`` ``` Dummies 10000 items Make dummies: 3.6226229667664 Compute changes: 0.18751001358032 Recompute changes: 0.23972296714783 ``` _note: `DateTime` does not have `__toString` so it ignored here_ #### ``Doctrine\DBAL\Types\Type::isValuesIdentical($val1, $val2)`` ``` Dummies 10000 items Make dummies: 3.6306829452515 Compute changes: 0.22055912017822 Recompute changes: 0.24937987327576 ``` The simple objects like `Point` do not make huge difference, it around the same with default. But more complex Objects can take some more time, that true. From my point of view `Doctrine\DBAL\Types\Type::isValuesIdentical($val1, $val2)` is good enough to accept (but still not perfect 😉 ) if I have missed something in my test, please tell me.
Author
Owner

@Ocramius commented on GitHub (Nov 8, 2017):

Hey Fedir,

Please use phpbench when writing tests, as it considers standard deviation
and other factors.

I see a 23% slower API there - that's not gonna be OK.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On Wed, Nov 8, 2017 at 9:15 PM, Fedir Zinchuk notifications@github.com
wrote:

I have made tests.
The full source of the test is there https://github.com/Fedik/
doctrine-changeset-test
(all time in seconds)
default

Dummies 10000 items
Make dummies: 3.6029109954834
Compute changes: 0.1790030002594
Recompute changes: 0.22904109954834 <02290%204109954834>

method_exists($orgValue, '__toString')

Dummies 10000 items
Make dummies: 3.6226229667664
Compute changes: 0.18751001358032
Recompute changes: 0.23972296714783 <02397%202296714783>

note: DateTime does not have __toString so it ignored here
Doctrine\DBAL\Types\Type::isValuesIdentical($val1, $val2)

Dummies 10000 items
Make dummies: 3.6306829452515
Compute changes: 0.22055912017822 <02205%205912017822>
Recompute changes: 0.24937987327576 <02493%207987327576>

The simple objects like Point do not make huge difference, it around the
same with default.
But more complex Objects can take some more time, that true.

From my point of view Doctrine\DBAL\Types\Type::isValuesIdentical($val1,
$val2) is good enough to accept.

if I have missed something in my test, please tell me.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/doctrine/doctrine2/issues/5542#issuecomment-342945485,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakBufPuBWKlnXk1arnQ6ziGktAqBYks5s0gvegaJpZM4Gym55
.

@Ocramius commented on GitHub (Nov 8, 2017): Hey Fedir, Please use phpbench when writing tests, as it considers standard deviation and other factors. I see a 23% slower API there - that's not gonna be OK. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ On Wed, Nov 8, 2017 at 9:15 PM, Fedir Zinchuk <notifications@github.com> wrote: > I have made tests. > The full source of the test is there https://github.com/Fedik/ > doctrine-changeset-test > *(all time in seconds)* > default > > Dummies 10000 items > Make dummies: 3.6029109954834 > Compute changes: 0.1790030002594 > Recompute changes: 0.22904109954834 <02290%204109954834> > > method_exists($orgValue, '__toString') > > Dummies 10000 items > Make dummies: 3.6226229667664 > Compute changes: 0.18751001358032 > Recompute changes: 0.23972296714783 <02397%202296714783> > > *note: DateTime does not have __toString so it ignored here* > Doctrine\DBAL\Types\Type::isValuesIdentical($val1, $val2) > > Dummies 10000 items > Make dummies: 3.6306829452515 > Compute changes: 0.22055912017822 <02205%205912017822> > Recompute changes: 0.24937987327576 <02493%207987327576> > > The simple objects like Point do not make huge difference, it around the > same with default. > But more complex Objects can take some more time, that true. > > From my point of view Doctrine\DBAL\Types\Type::isValuesIdentical($val1, > $val2) is good enough to accept. > > if I have missed something in my test, please tell me. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/doctrine/doctrine2/issues/5542#issuecomment-342945485>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAJakBufPuBWKlnXk1arnQ6ziGktAqBYks5s0gvegaJpZM4Gym55> > . >
Author
Owner

@DHager commented on GitHub (Nov 9, 2017):

Well, this an interesting ticket from the past. Looking at my previous post, am I correct in assuming that the use-case we're all talking here about involves checking the dirty/changed status for custom DBAL types that represent a column? In other words, the same problem causing the warning that "DateTime changes are detected by Reference"?

@Ocramius I'm not sure that the performance outlook is all that dire, because in practice the ORM can skip the method call for a majority of entity-properties.

Suppose there's are some new potential methods on Doctrine\DBAL\Types\Type:

  • useCustomDirtyCheck() should always return either true or false
  • isDirty($obj1, $obj2) only matters if custom check is used
  • markClean($obj1) only matters if custom check is used

The output from the first method can easily be cached, so that most cases (ex: StringType, BooleanType, IntegerType) only need the existing strict-quality test.

To sketch it out:

/** @var $type \Doctrine\DBAL\Types\Type **/
/** @var $type_has_custom_check boolean **/
if(!$type_has_custom_check){
    // This if-statement is how UnitOfWork currently operates
    if ($orgValue === $actualValue) { 
        continue;
    }
}else{    
    if ($type->isDirty($orgValue, $actualValue)) {
        continue;
    }
}

So ~95% of the time the only additional cost is checking a boolean value, one that is effectively a constant for the lifetime of the process.

The other 5% of the time, like when using MoneyType, the ORM knows it must delegate the job of figuring out whether the entity-property is dirty to MoneyType::isDirty($orgValue, $actualValue). (Obviously it's up to the designer of Money and MoneyType to make sure both of them work together correctly.)

P.S.: Originally I got stuck thinking of this as an equality test, but it really isn't, because both arguments to isDirty() could easily be the exact same object, and what we really want to know is whether that object was altered since it was loaded from the database. I also added markClean() which would need to be run after successfully updating/inserting the entities.

@DHager commented on GitHub (Nov 9, 2017): Well, this an interesting ticket from the past. Looking at my previous post, am I correct in assuming that the use-case we're all talking here about involves checking the dirty/changed status for custom DBAL types that represent a column? In other words, the same problem causing the warning that ["DateTime changes are detected by Reference"](http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/cookbook/working-with-datetime.html)? @Ocramius I'm not sure that the performance outlook is all that dire, because in practice the ORM can skip the method call for a majority of entity-properties. Suppose there's are some new potential methods on `Doctrine\DBAL\Types\Type`: * `useCustomDirtyCheck()` should always return either `true` or `false` * `isDirty($obj1, $obj2)` only matters if custom check is used * `markClean($obj1)` only matters if custom check is used The output from the first method can easily be cached, so that most cases (ex: `StringType`, `BooleanType`, `IntegerType`) only need the [existing strict-quality test](https://github.com/doctrine/doctrine2/blob/a7e13f89ccdf91aa9b06bf5d4be0addf9ad2ca40/lib/Doctrine/ORM/UnitOfWork.php#L696). To sketch it out: /** @var $type \Doctrine\DBAL\Types\Type **/ /** @var $type_has_custom_check boolean **/ if(!$type_has_custom_check){ // This if-statement is how UnitOfWork currently operates if ($orgValue === $actualValue) { continue; } }else{ if ($type->isDirty($orgValue, $actualValue)) { continue; } } So ~95% of the time the only additional cost is checking a boolean value, one that is effectively a constant for the lifetime of the process. The other 5% of the time, like when using `MoneyType`, the ORM knows it must delegate the job of figuring out whether the entity-property is dirty to `MoneyType::isDirty($orgValue, $actualValue)`. (Obviously it's up to the designer of `Money` and `MoneyType` to make sure both of them work together correctly.) **P.S.:** Originally I got stuck thinking of this as an equality test, but it really isn't, because both arguments to `isDirty()` could easily be the exact same object, and what we really want to know is whether that object was altered since it was loaded from the database. I also added `markClean()` which would need to be run after successfully updating/inserting the entities.
Author
Owner

@DHager commented on GitHub (Nov 9, 2017):

@Fedik My concern about isValuesIdentical is that while it works for immutable value-objects, it doesn't work for mutable types. Supposing Point was invented to be mutable, someone could go:

$entity->point1->travel($compassHeading, $distance)

And later on the isValuesIdentical() test would get the same object for both inputs, and falsely indicate that nothing needed to be saved.

@DHager commented on GitHub (Nov 9, 2017): @Fedik My concern about `isValuesIdentical` is that while it works for immutable value-objects, it doesn't work for mutable types. Supposing `Point` was invented to be mutable, someone could go: $entity->point1->travel($compassHeading, $distance) And later on the `isValuesIdentical()` test would get the same object for both inputs, and falsely indicate that nothing needed to be saved.
Author
Owner

@Majkl578 commented on GitHub (Dec 6, 2017):

Note: Issue with mutability also applies to Embeddables, but on entirely different level of implementation. If there's to be some kind of a Comparison API, it'd be good to share the core idea between Types and Embeddables.

@Majkl578 commented on GitHub (Dec 6, 2017): Note: Issue with mutability also applies to Embeddables, but on entirely different level of implementation. If there's to be some kind of a Comparison API, it'd be good to share the core idea between Types and Embeddables.
Author
Owner

@surelygroup commented on GitHub (Apr 16, 2019):

Any movement on this issue? Has any decision on the way forward been made?

@surelygroup commented on GitHub (Apr 16, 2019): Any movement on this issue? Has any decision on the way forward been made?
Author
Owner

@Ocramius commented on GitHub (Apr 16, 2019):

@surelygroup nothing moving for now.

@Ocramius commented on GitHub (Apr 16, 2019): @surelygroup nothing moving for now.
Author
Owner

@PowerKiKi commented on GitHub (Oct 25, 2020):

DHager's proposed solution in https://github.com/doctrine/orm/issues/5542#issuecomment-343024280 would solve our use-case too. And from a performance point of view it seems it could be reasonable.

While adding an extra check on each field will cost something, we should not forget the cost of the SQL query itself. In our use-case we have custom Point type. All entities with a field of that type will always execute and UPDATE statement. In many cases this is useless and it is a huge performance hit (compared to a function call).

The fact that there is always a changeset also happen to trip up our "last updated" mechanism. An entity with with a custom field of Point will always be marked as last updated "now", even if nothing actually changed. I guess there could be workarounds for that particular issue (by manually re-checking the changeset for actual changes), but it seems to me that Doctrine should not notify us of change if nothing is actually changed.

@Ocramius would you accept a PR (actually two, for DBAL and ORM) that implements DHager's solution ?

@PowerKiKi commented on GitHub (Oct 25, 2020): DHager's proposed solution in https://github.com/doctrine/orm/issues/5542#issuecomment-343024280 would solve our use-case too. And from a performance point of view it seems it could be reasonable. While adding an extra check on each field will cost _something_, we should not forget the cost of the SQL query itself. In our use-case we have custom Point type. All entities with a field of that type will **always** execute and `UPDATE` statement. In many cases this is useless and it is a huge performance hit (compared to a function call). The fact that there is always a changeset also happen to trip up our "last updated" mechanism. An entity with with a custom field of Point will always be marked as last updated "now", even if nothing actually changed. I guess there could be workarounds for that particular issue (by manually re-checking the changeset for actual changes), but it seems to me that Doctrine should not notify us of change if nothing is actually changed. @Ocramius would you accept a PR (actually two, for DBAL and ORM) that implements DHager's solution ?
Author
Owner

@PowerKiKi commented on GitHub (Oct 25, 2020):

For future reference, this problem, or a variation of it, came up a few times over the past 10 years: https://github.com/doctrine/orm/issues/3550, https://github.com/doctrine/orm/issues/7583, https://github.com/doctrine/orm/pull/7586, https://github.com/doctrine/orm/issues/7892, https://github.com/symfony/symfony/issues/11732

@PowerKiKi commented on GitHub (Oct 25, 2020): For future reference, this problem, or a variation of it, came up a few times over the past 10 years: https://github.com/doctrine/orm/issues/3550, https://github.com/doctrine/orm/issues/7583, https://github.com/doctrine/orm/pull/7586, https://github.com/doctrine/orm/issues/7892, https://github.com/symfony/symfony/issues/11732
Author
Owner

@allan-simon commented on GitHub (Jan 4, 2022):

As I understand this problem has no easy, one size fits them all solution, for people like me who can live with a workaround, is this one a good enough ?

add the comparison in the setter, i.e for mutable objects (like datetime or any custom type) having

// for briefness I have omitted the Doctrine annotations
class  MovieEntity
{
     public function setStartingTime(\Datetime $datetime): self
     {
           
           if ( (string) $datetime === (string)  $this->startingTime) { // or any comparison to check for 'value' equality
                 return $this;
           }
           $this->startingTime = clone $datetime; // with an additional if to check for reference to avoid always cloning
     }
}

Or is there some special magic in doctrine, that even calling the setter would marks it as dirty ?

If not and it seems good enough, I can start evangelizing this work around in my company's codebase (and even change the console make:entity of symfony to generate 'least surprising' setters when adding a Datetime/custom type field )

Of course one can still do $this->getStartingTime()->modify() but this can be check by static analysis like phpstan with phpstan-doctrine

@allan-simon commented on GitHub (Jan 4, 2022): As I understand this problem has no easy, one size fits them all solution, for people like me who can live with a workaround, is this one a good enough ? add the comparison in the setter, i.e for mutable objects (like datetime or any custom type) having ``` // for briefness I have omitted the Doctrine annotations class MovieEntity { public function setStartingTime(\Datetime $datetime): self { if ( (string) $datetime === (string) $this->startingTime) { // or any comparison to check for 'value' equality return $this; } $this->startingTime = clone $datetime; // with an additional if to check for reference to avoid always cloning } } ``` Or is there some special magic in doctrine, that even calling the setter would marks it as dirty ? If not and it seems good enough, I can start evangelizing this work around in my company's codebase (and even change the `console make:entity` of symfony to generate 'least surprising' setters when adding a Datetime/custom type field ) Of course one can still do $this->getStartingTime()->modify() but this can be check by static analysis like phpstan with phpstan-doctrine
Author
Owner

@PowerKiKi commented on GitHub (Jan 5, 2022):

If your real use-case is indeed Datetime, then a proper and permanent solution is to move your whole codebase to DateTimeImmutable, avoiding all possible issue at the language level. You might be interested in https://github.com/cakephp/chronos that supports immutable datetime as well as immutable date.

@PowerKiKi commented on GitHub (Jan 5, 2022): If your real use-case is indeed `Datetime`, then a proper and permanent solution is to move your whole codebase to `DateTimeImmutable`, avoiding all possible issue at the language level. You might be interested in https://github.com/cakephp/chronos that supports immutable [datetime](https://github.com/cakephp/chronos/blob/2.x/src/Chronos.php) as well as [immutable date](https://github.com/cakephp/chronos/blob/2.x/src/Date.php).
Author
Owner

@allan-simon commented on GitHub (Jan 5, 2022):

I have also the case with column of type "decimal" which seems to also be represented internally by an object,

for DateTimeImmutable, it only fix the issue of doing "->modify".

@allan-simon commented on GitHub (Jan 5, 2022): I have also the case with column of type "decimal" which seems to also be represented internally by an object, for DateTimeImmutable, it only fix the issue of doing "->modify".
Author
Owner

@PowerKiKi commented on GitHub (Jan 5, 2022):

DateTimeImmutable solves all cases, because to change the time value, you must replace the entire object, and thus Doctrine would work as expected. But be sure to use https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/types.html#date-immutable

DBAL decimal type is not an object, but a string, so you should not have the same issue with that type.

@PowerKiKi commented on GitHub (Jan 5, 2022): `DateTimeImmutable` solves all cases, because to change the time value, you _must_ replace the entire object, and thus Doctrine would work as expected. But be sure to use https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/types.html#date-immutable DBAL decimal type [is not an object, but a string](https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/types.html#decimal), so you should not have the same issue with that type.
Author
Owner

@allan-simon commented on GitHub (Jan 5, 2022):

DateTimeImmutable solves all cases, because to change the time value, you must replace the entire object, and thus Doctrine would work as expected. But be sure to use https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/types.html#date-immutable

no , my issue is spefically that putting the same point in time , create two objects (especially if it is immutable), even if both are "first january of 2022 at midnight" , causing unnecessary UPDATE queries even if the date has not change (for example you create a scrapper of the IMDB movie database, if you have a field release date, this field will cause your entity to be considered dirty as soon as you set) ,

DBAL decimal type is not an object, but a string, so you should not have the same issue with that type.

weird because that's the other type causing this issue on our side, could it be due to formating i,e we set "5" and doctrine has "5.00" ? so they are no === even though they are the same fixed point number ?
because for numeric-string one should use bccomp or equivalent

@allan-simon commented on GitHub (Jan 5, 2022): > DateTimeImmutable solves all cases, because to change the time value, you must replace the entire object, and thus Doctrine would work as expected. But be sure to use https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/types.html#date-immutable no , my issue is spefically that putting the same point in time , create two objects (especially if it is immutable), even if both are "first january of 2022 at midnight" , causing unnecessary `UPDATE` queries even if the date has not change (for example you create a scrapper of the IMDB movie database, if you have a field release date, this field will cause your entity to be considered dirty as soon as you set) , > DBAL decimal type is not an object, but a string, so you should not have the same issue with that type. weird because that's the other type causing this issue on our side, could it be due to formating i,e we set "5" and doctrine has "5.00" ? so they are no === even though they are the same fixed point number ? because for numeric-string one should use `bccomp` or equivalent
Author
Owner

@PowerKiKi commented on GitHub (Jan 5, 2022):

Sorry I got it all backwards. Of course you are right, DateTimeImmutable would not solve your issue.

@PowerKiKi commented on GitHub (Jan 5, 2022): Sorry I got it all backwards. Of course you are right, DateTimeImmutable would not solve your issue.
Author
Owner

@talkinnl commented on GitHub (Sep 30, 2022):

This issue currently has a broad scope, which might not help in resolving it.

Maybe perfect is the enemy of good here: Fixing all objects and providing some hooks arch etc is hard, so maybe it would be a start to fix native PHP object behavior first.
I think it'd already be a nice improvement if this issue is specifically fixed for DateTimeImmutable.

@talkinnl commented on GitHub (Sep 30, 2022): This issue currently has a broad scope, which might not help in resolving it. Maybe perfect is the enemy of good here: Fixing all objects and providing some hooks arch etc is hard, so maybe it would be a start to fix native PHP object behavior first. I think it'd already be a nice improvement if this issue is specifically fixed for DateTimeImmutable.
Author
Owner

@Ocramius commented on GitHub (Sep 30, 2022):

Send a patch: @DHager's analysis on happy path scenario makes sense, but it needs practical implementation and benchmarks (committed).

@Ocramius commented on GitHub (Sep 30, 2022): Send a patch: @DHager's analysis on happy path scenario makes sense, but it needs practical implementation and benchmarks (committed).
Author
Owner

@arno14 commented on GitHub (Oct 15, 2022):

There are currently many Use-cases where the data retrieved from the database is treated as an object,
and the fact that values are compared by reference is problematic.
(Every developper using Doctrine has already wasted a few hours on this problem)

  • Datetime Object (representing a date and/or time)
  • Geographical datas
  • Object serialized in json or serialized php value
  • ...

using DateTimeImmutable or Value Object can solve some problems, but it always require extra code.

I started a PR (https://github.com/doctrine/orm/pull/10137) which proposes to override the behavior with this interface:

interface ChangeDetector
{
    /**
     * How the UnitOfWork should keep trace of the original value
     * @param mixed $originalValue
     * @return mixed
     */
    public function copyOriginalValue(&$originalValue);

    /**
     * Whether or not the two value must be considered as different and trigger an UPDATE query
     * @param mixed $value
     * @param mixed $originalValue
     */
    public function isChanged($value, $originalValue): bool;
}

It can also be a starting point for the proposition of managing this in the Doctrine DBAL type:
to determine the change detection strategy, the following tests will occurs:

  • does the annotation mapping of the field includes attribute "changeDetector"?
  • does the DBAL\Type provide a custom changeDetector ?
  • If none is provided, the current behaviour (by reference) is used.

There are no breaking change with this solution:

Does this seem a good solution and should I continue this PR?

@arno14 commented on GitHub (Oct 15, 2022): There are currently many Use-cases where the data retrieved from the database is treated as an object, and the fact that values are compared by reference is problematic. (Every developper using Doctrine has already wasted a few hours on this problem) - Datetime Object (representing a date and/or time) - Geographical datas - Object serialized in json or serialized php value - ... using DateTimeImmutable or Value Object can solve some problems, but it always require extra code. I started a PR (https://github.com/doctrine/orm/pull/10137) which proposes to override the behavior with this interface: ``` interface ChangeDetector { /** * How the UnitOfWork should keep trace of the original value * @param mixed $originalValue * @return mixed */ public function copyOriginalValue(&$originalValue); /** * Whether or not the two value must be considered as different and trigger an UPDATE query * @param mixed $value * @param mixed $originalValue */ public function isChanged($value, $originalValue): bool; } ``` It can also be a starting point for the proposition of managing this in the Doctrine DBAL type: to determine the change detection strategy, the following tests will occurs: - does the annotation mapping of the field includes attribute "changeDetector"? - does the DBAL\Type provide a custom changeDetector ? - If none is provided, the current behaviour (by reference) is used. There are no breaking change with this solution: - the dev can choose to use a custom strategy - the extension developer library could choose to switch globally on an other strategy (example: https://github.com/dunglas/doctrine-json-odm/issues/21#issuecomment-339360519) Does this seem a good solution and should I continue this PR?
Author
Owner

@spackmat commented on GitHub (Feb 5, 2023):

I like the proposal from @arno14 as it looks like a reasonable way to implement something like that from a user's perspective.

(Every developper using Doctrine has already wasted a few hours on this problem)

Oh yes, I also wasted some hours scratching my head what was wrong with my implementation, as my previous immutable siblings worked just fine before. I have a lot of serialized JSON custom types and most of them are immutable, but some are mutable and those already implement an internal dirty state that an Interface could easily expose.

For now I do something like this as a workaround, if anyone is looking for a quick solution unless that underlying problem is eventually solved in any way:

public function addCustomTypeEntry(CustomTypeEntry $customTypeEntry): self
{
    // where $this->customTypeProperty is a mutable object holding a list of CustomTypeEntry objects serialized as JSON
    $this->customTypeProperty = (clone $this->customTypeProperty)->addEntry($customTypeEntry);
    return $this;
}
@spackmat commented on GitHub (Feb 5, 2023): I like the proposal from @arno14 as it looks like a reasonable way to implement something like that from a user's perspective. > (Every developper using Doctrine has already wasted a few hours on this problem) Oh yes, I also wasted some hours scratching my head what was wrong with my implementation, as my previous immutable siblings worked just fine before. I have a lot of serialized JSON custom types and most of them are immutable, but some are mutable and those already implement an internal dirty state that an Interface could easily expose. For now I do something like this as a workaround, if anyone is looking for a quick solution unless that underlying problem is eventually solved in any way: ```` php public function addCustomTypeEntry(CustomTypeEntry $customTypeEntry): self { // where $this->customTypeProperty is a mutable object holding a list of CustomTypeEntry objects serialized as JSON $this->customTypeProperty = (clone $this->customTypeProperty)->addEntry($customTypeEntry); return $this; } ````
Author
Owner

@sylfabre commented on GitHub (Oct 1, 2024):

@Ocramius does the newest versions of dbal and orm make it easier to implement?

@sylfabre commented on GitHub (Oct 1, 2024): @Ocramius does the newest versions of dbal and orm make it easier to implement?
Author
Owner

@Ocramius commented on GitHub (Oct 1, 2024):

No: conversions still very expensive.

@Ocramius commented on GitHub (Oct 1, 2024): No: conversions still very expensive.
Author
Owner

@arno14 commented on GitHub (Dec 5, 2024):

Some times ago, I have been working on this issue,
I think it may be usefull to provide a feeback about what I learned:

On this branch:
https://github.com/arno14/doctrine-orm/tree/5542_override_the_way_change_is_computed_by_uow
I implemented a new interface ChangeDetector, allowing different strategy for detecting change.

After reflexion, I realized that any process done if the ChangeDetector would be more or less
what is executed in Doctrine Type class (convertToDatabaseValue and comparing some string values instead of Object value).

So I also created this branch : https://github.com/arno14/doctrine-orm/tree/5542_compare_value_by_database_value
which uses the database value to detect changes.

But it is clearly not optimized because the process is that :
When fetching an entity :

  • on the DBAL Layer the query is executed and call to Type::convertToPhpValue is executed
  • theses php values are stored in UnitOfWork::$originalEntityData
  • these php values are reconverted to database value via Type::convertToDatabaseValue and stored in UnitOfWork::$originalRawData
  • the entity is populated with the phpvalues

When calling Flush :

  • The ChangeSet is computed based on a conversion of the entity attributes via Type::convertToDatabaseValue and the value stored in UnitOfWork::$originalRawData
  • If this computation detects a change, the phpValues are passed to the DBAL Layer
  • The DBAL Layer execute Type::convertToDatabaseValue and run the SQL query

So Type::convertToPhpValue and Type::convertToDatabaseValue are executed twice for the same datas

So to resolve this very old issue, I think it would be necessary to rethink the way original datas are stored,
in order to have this process:

When fetching an entity :

  • on the DBAL Layer the query is executed and database values are retrieved
  • theses database values are stored in UnitOfWork::$originalEntityData
  • database values are converted to php value and used to populate the entity

When calling Flush :

  • the php value is converted to database value
  • this database value is compared to the one in UnitOfWork::$originalEntityData
  • if values are differents, the DBAL Layer run the SQL query with the newly converted database value

But with this solution, the following problems would occurs:

  • That would produce a B.C. break,
    specially for Doctrine extensions hooking on the Flush event and analysing the entity changeSet.
    because the changeSet would then contains the database value, not the php ones.
  • Increasing the memory usage for large database value like blob
  • The UnitOfWork::$originalEntityData would be a mix of database and php values (Collection)

The problem of the BC break could be minimized by adopting a new FieldMapping option "detectChangeByDatabaseValue".
Default value would be false reproducing the actual behavior (original data stored as Php value and comparisation based on php values also).

@arno14 commented on GitHub (Dec 5, 2024): Some times ago, I have been working on this issue, I think it may be usefull to provide a feeback about what I learned: On this branch: https://github.com/arno14/doctrine-orm/tree/5542_override_the_way_change_is_computed_by_uow I implemented a new interface ChangeDetector, allowing different strategy for detecting change. After reflexion, I realized that any process done if the ChangeDetector would be more or less what is executed in Doctrine Type class (convertToDatabaseValue and comparing some string values instead of Object value). So I also created this branch : https://github.com/arno14/doctrine-orm/tree/5542_compare_value_by_database_value which uses the database value to detect changes. But it is clearly not optimized because the process is that : When fetching an entity : - on the DBAL Layer the query is executed and call to Type::convertToPhpValue is executed - theses php values are stored in UnitOfWork::$originalEntityData - these php values are reconverted to database value via Type::convertToDatabaseValue and stored in UnitOfWork::$originalRawData - the entity is populated with the phpvalues When calling Flush : - The ChangeSet is computed based on a conversion of the entity attributes via Type::convertToDatabaseValue and the value stored in UnitOfWork::$originalRawData - If this computation detects a change, the phpValues are passed to the DBAL Layer - The DBAL Layer execute Type::convertToDatabaseValue and run the SQL query So Type::convertToPhpValue and Type::convertToDatabaseValue are executed twice for the same datas So to resolve this very old issue, I think it would be necessary to rethink the way original datas are stored, in order to have this process: When fetching an entity : - on the DBAL Layer the query is executed and database values are retrieved - theses database values are stored in UnitOfWork::$originalEntityData - database values are converted to php value and used to populate the entity When calling Flush : - the php value is converted to database value - this database value is compared to the one in UnitOfWork::$originalEntityData - if values are differents, the DBAL Layer run the SQL query with the newly converted database value But with this solution, the following problems would occurs: - That would produce a B.C. break, specially for Doctrine extensions hooking on the Flush event and analysing the entity changeSet. because the changeSet would then contains the database value, not the php ones. - Increasing the memory usage for large database value like blob - The UnitOfWork::$originalEntityData would be a mix of database and php values (Collection) The problem of the BC break could be minimized by adopting a new FieldMapping option "detectChangeByDatabaseValue". Default value would be false reproducing the actual behavior (original data stored as Php value and comparisation based on php values also).
Author
Owner

@mkoskl commented on GitHub (Jul 1, 2025):

What's the status of this?

We run into this same problem with DateTimeImmutable instances. The instance changes, but the actual date stays the same and still Doctrine thinks the database value has been changed.

@mkoskl commented on GitHub (Jul 1, 2025): What's the status of this? We run into this same problem with DateTimeImmutable instances. The instance changes, but the actual date stays the same and still Doctrine thinks the database value has been changed.
Author
Owner

@Ocramius commented on GitHub (Jul 1, 2025):

@mkoskl in order for this issue to be resolved, somebody would need to provide an implementation following @arno14's research, whilst retaining decent performance on the comparison of in-memory values.

It's not trivial, and AFAIK, nobody is working on it actively: you'll need to step up yourself, if you want it done, and it may still fail due to the additional overhead this could introduce.

@Ocramius commented on GitHub (Jul 1, 2025): @mkoskl in order for this issue to be resolved, somebody would need to provide an implementation following @arno14's research, whilst retaining decent performance on the comparison of in-memory values. It's not trivial, and AFAIK, nobody is working on it actively: you'll need to step up yourself, if you want it done, and it may still fail due to the additional overhead this could introduce.
Author
Owner

@allan-simon commented on GitHub (Jul 1, 2025):

@mkoskl in our project we worked around the project by adding a comparison in the setter of these objects (datetime/decimal etc.) that does not change the php value if the database value is the same

something like

function setDate($x) {
      if ($x != $this->date) {
            $this->date = $x;
      }  
      return $this;
}

it requires some discipline but with some semgrep rules (or phpstan custom rules) you can easily automatically find them before it hits production

@allan-simon commented on GitHub (Jul 1, 2025): @mkoskl in our project we worked around the project by adding a comparison in the setter of these objects (datetime/decimal etc.) that does not change the php value if the database value is the same something like ``` function setDate($x) { if ($x != $this->date) { $this->date = $x; } return $this; } ``` it requires some discipline but with some `semgrep` rules (or phpstan custom rules) you can easily automatically find them before it hits production
Author
Owner

@mkoskl commented on GitHub (Jul 1, 2025):

@Ocramius ok, thanks for answer!

One thing that came to my mind is that would it be solved if on PHP side correct comparision operator would be implemented? Like PHPs internal version of $date1 === $date2 would compare string versions of those DateTime(Immutable) objects?

@mkoskl commented on GitHub (Jul 1, 2025): @Ocramius ok, thanks for answer! One thing that came to my mind is that would it be solved if on PHP side correct comparision operator would be implemented? Like PHPs internal version of $date1 === $date2 would compare string versions of those DateTime(Immutable) objects?
Author
Owner

@mkoskl commented on GitHub (Jul 1, 2025):

@allan-simon yea, it would work. Only thing is that we have quite many entities and almost all of them have added / modified dates.

Maybe replacing entity-specific setters with some kind of timestampable trait would suffice?

@mkoskl commented on GitHub (Jul 1, 2025): @allan-simon yea, it would work. Only thing is that we have quite many entities and almost all of them have added / modified dates. Maybe replacing entity-specific setters with some kind of timestampable trait would suffice?
Author
Owner

@mkoskl commented on GitHub (Jul 1, 2025):

Hm, one more note. It seems DateTime objects can be compared, but with not strict operator (===), but with double-equals (==) operator.

Edit: So what would be consequences of allowing comparision by loose operator?

@mkoskl commented on GitHub (Jul 1, 2025): Hm, one more note. It seems DateTime objects can be compared, but with not strict operator (===), but with double-equals (==) operator. Edit: So what would be consequences of allowing comparision by loose operator?
Author
Owner

@mpdude commented on GitHub (Oct 9, 2025):

Also may apply to/impact Criteria matching/filtering API, where equality checks and in-memory sorting rely on comparisons that are currently doing === checks (e. g. in \Doctrine\Common\Collections\Expr\ClosureExpressionVisitor::sortByField).

@mpdude commented on GitHub (Oct 9, 2025): Also may apply to/impact Criteria matching/filtering API, where equality checks and in-memory sorting rely on comparisons that are currently doing `===` checks (e. g. in `\Doctrine\Common\Collections\Expr\ClosureExpressionVisitor::sortByField`).
Author
Owner

@mpdude commented on GitHub (Oct 9, 2025):

We cannot reasonably require objects to implement a given interface for implementing comparisons (even if only for the ORM purpose). For example with DateTime types, those objects may be created by libraries like Carbon or by some clock mocking mechanism – and we cannot expect those libraries to implement our interfaces for the purpose of comparison in the ORM.

So, comparison would need to be something that can be attached from the outside.

@mpdude commented on GitHub (Oct 9, 2025): We cannot reasonably require objects to implement a given interface for implementing comparisons (even if only for the ORM purpose). For example with DateTime types, those objects may be created by libraries like [Carbon](https://carbon.nesbot.com/) or by some clock mocking mechanism – and we cannot expect those libraries to implement our interfaces for the purpose of comparison in the ORM. So, comparison would need to be something that can be attached from the outside.
Author
Owner

@mpdude commented on GitHub (Oct 9, 2025):

When having two different date time objects that have different dates, but the same time, and I update a time typed column from the one object to the other... should that be considered "equal" values, so the derived changeset is empty?

@mpdude commented on GitHub (Oct 9, 2025): When having two different date time objects that have different dates, but the same time, and I update a `time` typed column from the one object to the other... should that be considered "equal" values, so the derived changeset is empty?
Author
Owner

@theofidry commented on GitHub (Oct 9, 2025):

I think having a comparator API would be necessary. The current behaviour equivalent would be to have one comparator that compares objects strictly, then you could have another that compares a given list of object classes loosely and people could register then own and use their own API.

As a workaround what I am doing is having a ValueObjectEqualitySupportListener pre-flush listener that goes through the object properties. For each it picks the original value and if they implement our API and are equal (ValueObject::equals()), then we use the underlying reflector to set the original value.

But as you can imagine it's not ideal, and relies on internal API (PropertyAccessor is @internal)


I think any solution is bound to have a performance degradation hence I think it would be more interesting to provide an extension point to allow one to do their things, but too keep the current behaviour as is, as it is the most optimal performance-wise.

@theofidry commented on GitHub (Oct 9, 2025): I think having a comparator API would be necessary. The current behaviour equivalent would be to have one comparator that compares objects strictly, then you could have another that compares a given list of object classes loosely and people could register then own and use their own API. As a workaround what I am doing is having a `ValueObjectEqualitySupportListener` pre-flush listener that goes through the object properties. For each it picks the original value and if they implement our API and are equal (`ValueObject::equals()`), then we use the underlying reflector to set the original value. But as you can imagine it's not ideal, and relies on internal API (`PropertyAccessor` is `@internal`) <hr> I think any solution is bound to have a performance degradation hence I think it would be more interesting to provide an extension point to allow one to do their things, but too keep the current behaviour as is, as it is the most optimal performance-wise.
Author
Owner

@mpdude commented on GitHub (Oct 9, 2025):

Experiment in #12215.

Problem: We also need to recognize changes made to mutable objects, but (to my knowledge, might be wrong) we do not keep clones of the previous states/values around in the UoW.

@mpdude commented on GitHub (Oct 9, 2025): Experiment in #12215. Problem: We also need to recognize changes made to mutable objects, but (to my knowledge, might be wrong) we do not keep clones of the previous states/values around in the UoW.
Author
Owner

@theofidry commented on GitHub (Oct 9, 2025):

You can from the UoW:

$originalEntityData = $objectManager->getUnitOfWork()->getOriginalEntityData($entity);
@theofidry commented on GitHub (Oct 9, 2025): You can from the UoW: ```php $originalEntityData = $objectManager->getUnitOfWork()->getOriginalEntityData($entity); ```
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: doctrine/archived-orm#4931