Allow to not Compare Object Types by reference

[beberlei]

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.

[Ocramius]

I proposed comparing the DB-side representation in the past, but it may be
expensive...
On Dec 10, 2015 11:23, "Benjamin Eberlei" [email protected] 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
#5542.

[beberlei]

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

[DHager]

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.

[Ocramius]

@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).

[DHager]

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

[Steveb-p]

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

[Fedik]

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?

2. 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.

[Ocramius]

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.

[Fedik]

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]

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.

[Ocramius]

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 ***@***.***> 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 <#5542 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakBufPuBWKlnXk1arnQ6ziGktAqBYks5s0gvegaJpZM4Gym55> .

[DHager]

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]

@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.

[Majkl578]

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.

[surelygroup]

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

[Ocramius]

@surelygroup nothing moving for now.

[PowerKiKi]

DHager's proposed solution in #5542 (comment) 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]

For future reference, this problem, or a variation of it, came up a few times over the past 10 years: #3550, #7583, #7586, #7892, symfony/symfony#11732

[allan-simon]

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

[PowerKiKi]

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.

[allan-simon]

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".

[PowerKiKi]

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.

[allan-simon]

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

[PowerKiKi]

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

[talkinnl]

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.

[Ocramius]

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

[arno14]

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 (#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: POPOs data is not being persisted dunglas/doctrine-json-odm#21 (comment))

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

[spackmat]

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;
}

[sylfabre]

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

[Ocramius]

No: conversions still very expensive.

[arno14]

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).