Experiment: Use comparator functions from a global registry for changeset detection

[mpdude]

This is an experiment towards #5542. Can we have something to do comparison of objects to get away from === checks in changeset computation, but also in Criteria comparisons and sorting (in ClosureExpressionVisitor)?

We cannot expect users to implement interfaces for this in their objects. For example, users might be bringing their own subclasses of DateTime (like Carbon or Chronos libraries) or get their DateTime objects e. g. from clock mocking libraries. So, these objects might be coming from very differnet places and the classes used or created cannot all implement Doctrine interfaces.

Thus, the mechanism for bringing in the comparisons has to be attached or brought to the objects from the outside.

A global-state registry is not nice, but a very simple approach to start the discussion.

In this PR, comparators are registered based on class or interface names. Registration happens in order, and the first one matching an object at hand that returns a result different from null will win.

[mpdude]

As it turns out, the problem is not only figuring out that two different objects may acutally represent the same value.

Even more complicated is the case of detecting changes to mutable objects.

In the UoW, we keep the object (not clones of it) as the previous value, so for mutable objects, we have nothing to compare against.

[mpdude]

• Comparators are on the hot path of writing data
• Need not only be able to compare objects of the same class but of different classes as well
• Order of registration should not be relevant
• Finding the comparator "closest" to a given class may be O(n) in the number of comparators

[mpdude]

We may get away by emphasizing that we do not support mutable objects and/or that users have to deal with the quirk (as since the beginning) that they need to replace objects themselves.

But, working with immutable objects (like DateTimeImmutable) still may cause new instances of the same value be created, e. g. Symfony Form system binding data, putting in new DateTimeImmutable with new values.

-> In this case we'd still benefit from not considering this a change.

[stof]

The case of tracking changes for mutable value objects has never been supported by the ORM ever since 2.0. It always required you to clone the object before mutation and then assign that clone. So this case of mutable value object is a totally different feature than detecting equivalent immutable value objects to avoid over-writing in the flush. And that's probably a feature we should not even attempt to implement, keeping the rule that you need to not mutate the persisted value object (for which we should recommend using immutable ones then).

[beberlei]

Great start capturing most of our discussion. I have added some micro optimizations and thoughts.

[beberlei]

Suggested change

	if (is_object($orgValue) && is_object($actualValue) && ComparatorRegistry::compare($orgValue, $actualValue) === 0) {
	if (is_object($orgValue) && isset(ComparatorRegsitry::$calbacks[$orgValue::class]) && ComparatorRegistry::compare($orgValue, $actualValue) === 0) {

[beberlei]

Should do an isset before calling a function, that is much faster.

[beberlei]

public for fast access

[beberlei]

Why not directly access the callback?

self::$callbacks[$a::class]

[mpdude]

To support callbacks at a generic level, eg for \DateTimeInterface

[beberlei]

Its too expensive for too little gain, in changeset computation we are already loooping classes, all their instances, and all their properties, are 3 levels deep in loops here. Adding another one should not be done lightly.

[mpdude]

Then we cannot have comparators registered for types like \DateTimeInterface in order to catch everything implementing that, need to register all the potential class names

[mpdude]

And apart from is_object checks, we add the overhead only when looking at field values that are objects and where the identity changed

[beberlei]

While i get the sentiment to think this should be resettable, the comparison functions for objects are mathemtics, and mathematics is universal, it should not be necessasry to configure, overwrite, or reset them.

[mpdude]

During tests, to unregister them?

[beberlei]

Maybe, but your tests always add exactly the same callback, and the resetting does not add value there either.

If this were a core feature and classes would implement the interfaces and the method, you wouldn't be able to "unset" the class to use an interface and its implementation either, or replace it.

[mpdude]

Not gonna fight for this one :)

[beberlei]

• Comparators are on the hot path of writing data

but in the hot path of changeset computation.

[mpdude]

Note to self:

• Make sure we don't apply comparator logic to fields that are associations (contain entities), only fields that are "values" from the ORM point of view
• Look into how this applies to embeddables (comparing entire embeddables vs. comparing them at the field level?)