mirror of
https://github.com/doctrine/orm.git
synced 2026-03-23 22:42:18 +01:00
Allow to not Compare Object Types by reference #4931
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
@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:
@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.
@DHager commented on GitHub (Dec 10, 2015):
I expect that if someone has something like a
DateTimeorColorCode, 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
EntityManageralways has an, uhm,EntityScalarComparatorentities, 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
EntityScalarComparatoris where you might , say, have an option to change howDateTimeis compared, or a mechanism to chain/overlay another comparison service to support your own custom objects.@Ocramius commented on GitHub (Dec 10, 2015):
Possibly, but I'd still look at it after @guilhermeblanco is done with digging through JPA's field mappings.
The idea is to generate specific comparators per entity (you pretty much always compare entity to entity, not just single fields).
@DHager commented on GitHub (Dec 11, 2015):
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
BitMaskDBAL type that saves/loads from an SQL integer column. There are two possible failure-cases:BitMaskinstance 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".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:
But for that particular class of field-representing-object, the designer might want custom behavior more like:
@Steveb-p commented on GitHub (Apr 5, 2016):
Probably affects objects serialization: http://stackoverflow.com/questions/30193351/how-to-update-doctrine-object-type-field
@Fedik commented on GitHub (Nov 7, 2017):
I have a couple custom types, and it a bit annoying 😄 , so I got 2 ideas:
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.then UOW just do
$type->compareValues($old, $new);Is it expensive?
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 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.
@Fedik commented on GitHub (Nov 7, 2017):
okay, more complicated than I thought
what it means? sorry, not very understoodhm, okay I think I understood
yeah that can be more expansive, because
__toStringcan contain extra complex logic,then
Doctrine\DBAL\Types\Typelooks more simple betwen these both, but performance still depend from what developer put insideDoctrine\DBAL\Types\MyCustomType::compareValues().hm, okay, not perfect
@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
method_exists($orgValue, '__toString')note:
DateTimedoes not have__toStringso it ignored hereDoctrine\DBAL\Types\Type::isValuesIdentical($val1, $val2)The simple objects like
Pointdo 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 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:
@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 eithertrueorfalseisDirty($obj1, $obj2)only matters if custom check is usedmarkClean($obj1)only matters if custom check is usedThe 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:
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 toMoneyType::isDirty($orgValue, $actualValue). (Obviously it's up to the designer ofMoneyandMoneyTypeto 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 addedmarkClean()which would need to be run after successfully updating/inserting the entities.@DHager commented on GitHub (Nov 9, 2017):
@Fedik My concern about
isValuesIdenticalis that while it works for immutable value-objects, it doesn't work for mutable types. SupposingPointwas invented to be mutable, someone could go:And later on the
isValuesIdentical()test would get the same object for both inputs, and falsely indicate that nothing needed to be saved.@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.
@surelygroup commented on GitHub (Apr 16, 2019):
Any movement on this issue? Has any decision on the way forward been made?
@Ocramius commented on GitHub (Apr 16, 2019):
@surelygroup nothing moving for now.
@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
UPDATEstatement. 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):
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 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
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:entityof 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 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 toDateTimeImmutable, 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 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".
@PowerKiKi commented on GitHub (Jan 5, 2022):
DateTimeImmutablesolves 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-immutableDBAL decimal type is not an object, but a string, so you should not have the same issue with that type.
@allan-simon commented on GitHub (Jan 5, 2022):
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
UPDATEqueries 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) ,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
bccompor equivalent@PowerKiKi commented on GitHub (Jan 5, 2022):
Sorry I got it all backwards. Of course you are right, DateTimeImmutable would not solve your issue.
@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.
@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).
@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)
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:
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:
There are no breaking change with this solution:
(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 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.
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:
@sylfabre commented on GitHub (Oct 1, 2024):
@Ocramius does the newest versions of dbal and orm make it easier to implement?
@Ocramius commented on GitHub (Oct 1, 2024):
No: conversions still very expensive.
@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 :
When calling Flush :
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 :
When calling Flush :
But with this solution, the following problems would occurs:
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.
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).
@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.
@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.
@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
it requires some discipline but with some
semgreprules (or phpstan custom rules) you can easily automatically find them before it hits production@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):
@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):
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?
@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):
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):
When having two different date time objects that have different dates, but the same time, and I update a
timetyped column from the one object to the other... should that be considered "equal" values, so the derived changeset is empty?@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
ValueObjectEqualitySupportListenerpre-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 (
PropertyAccessoris@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.
@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.
@theofidry commented on GitHub (Oct 9, 2025):
You can from the UoW: