Revert unnecessary breaking change to Item::equals

[thiemowmde]

This mostly reverts the unnecessary breaking changes in #615. The title of that pull request was "Add equals to EntityDocument", and this is mostly what this patch now does, when you compare the code with the previous 4.4 release: The Comparable interface moves from the deprecated abstract base class Entity up to the EntityDocument interface. That's all.

This patch also cleans up the release notes (extensively updated in 2df96fa, thanks a lot for that, @Benestar) and adds some missing regression tests (I tried and these tests run with 4.4).

Bug: T125636

[Benestar]

I'd suggest to split this up into the uncontroversial changes to release notes and the added tests on the one hand and the changes to EntityDocument on the other hand.

Pinging @brightbyte as I remember he had arguments against making EntityDocument extend Comparable.

[Benestar]

I personally don't really bother as long as we have equals in some way on EntityDocument.

[thiemowmde]

I can split this, but please note that this reverts the controversial change in #615.

[thiemowmde]

Rebased.

[JeroenDeDauw]

+1. I'm fine with either approach. The old one clearly did not cause real issues, and I don't see the new one causing them either. Sure, each of them has its own little advantage, though arguing over this seems hardly worth peoples time.

[thiemowmde]

This is approaching the "problem" (if it ever was a problem, in two years I have never heard of it causing any problems) from the wrong side. The correct way is to deprecate the interface in DataValues, and then remove it everywhere (currently used in about 50 classes and subclasses, most of them here in DataModel) in one go. It's confusing and just unnecessary to remove it from a random class just because somebody feels like, with almost no discussion and no investigation where this is used and what hassle this random breaking change will cause.

With this patch there is zero breaking change to Item::equals and Property::equals. For now. This may change later when we drop our custom Comparable interface. Or not, because even dropping the interface does not mean any equals method must change. They are all fine as they are.

Equals methods not being symmetric when subclassing is involved is an actual, real-world problem that caused actual trouble. I spend some time working on this and solved this in all cases that are known to have subclasses, see for example SnakObject::equals. The fun bit is: This does not change with what we are discussing here. This discussion is about false negatives, not about false positives. False positives are a problem. But under which circumstances can a false negative - an equals method returning false - be a problem?

[thiemowmde]

Pinging @brightbyte.

[JeroenDeDauw]

The correct way is to deprecate the interface in DataValues, and then remove it everywhere

That makes no sense to me whatsoever.

[thiemowmde]

I'm asking for a decision to not use this interface any more, backed by a discussion that involved the team. A @deprecated tag would document the outcome of such a discussion.

But as I said, not even that would mean equals methods must have restricting type hints. That's a separate discussion.

[Benestar]

I can follow Thiemo's argumentation here that we should for one not force a breaking change to the equals method and furthermore be consistent with other classes' equals methods. If we decide to ditch Comparable once (not sure it is worth thinking about that because we gain literally nothing from that) we should do it at the same time for all classes in the DataModel.

What I miss in this patch now is that the definition of equals is not written in EntityDocument anymore. I would like to see somewhere in EntityDocument that equals is supposed to ignore the id so that it becomes part of its contract.

[thiemowmde]

we gain literally nothing from that

I disagree. It's a common, consistent contract for all classes implementing this interface. You can see implements Comparable in the class header, and tools like PHPStorm will conveniently show this information in multiple places. You don't have to search for a method. You don't have to think about how the method may be called. Could be isEqual or equalTo. (Yes, this really exists, and is actually very common, e.g. in PHPUnit.) You know, from the class header alone, how the method is called, how to use it and how it will act.

On the other hand, with a restricting type hint, you loose any guarantee, because there can not be a common interface for this any more. How do you make a guarantee that an object does have an equals method then? You loose so much information, convenience and comfort, even if there is no instanceof Comparable in your code base, but what do you win? The only advantage is that your IDE will tell you when you are calling equals with an odd type. How often did that happened to you in the past years? Really. Tell me.

[Benestar]

You misunderstood my statement. I was about to say that we gain nothing from removing the interface, no matter how much we argue whether it is benefitial or not. I'd suggest to just leave it there until we find "real" reasons to change something.