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

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

[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 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]

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

[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 (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?

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