ListPatcher: use non-strict comparison when removing objects

[manicki]

Background: discovered in Wikibase when trying to apply a patch removing an ItemId object from the list of ItemId objects. This failed if the "same" ID was in the list, but it was not technically the same object in memory.

While discussing the issue with @wiese and @jakobw, we've mentioned that further extension would be to allow to provide a function/object that would allow custom object comparison if something more sophisticated than PHP's native object non-strict comparison (i.e. all attribute and their values comparison) is needed. Not implemented here as there does not seem to be a use for such extension yet.

[manicki]

Also to be noted that ListDiffer's StrictArrayComparer does the similar thing for objects, so this change seem to be somewhat consistent.

[thiemowmde]

There might be a pretty significant misunderstanding going on here: To my knowledge diffs (as specified by this component) are not meant to hold objects, but only trivial values such as strings, as well as arrays build from trivial values. Basically: only values that can survive a json_encode and json_decode roundtrip are allowed.

In other words: Diffs are meant to be serializable.

I know the diffing classes in the Lexeme extensions are ignoring this limitation since diffing and patching was introduced there, which might explain why you run into this issue now.

I suggest to fix this in the Lexeme codebase, and not fiddle with this component to much.

Also notice this particular patch against master will be of no use for Wikibase, as the master branch of this component does not support PHP 5 any more since #74. You might want to branch or even fork this codebase.

[manicki]

Good that you say so, as I without any doubt not really familiar with this library, and I appreciate getting more information on it. And I actually had initially thought that it is meant to only be used with non-objects. But then I have e.g. found https://github.com/wmde/Diff/blob/master/src/ArrayComparer/StrictArrayComparer.php#L39 and it seemed to me to make sense to have a similar counterpart in ListPatcher. I am not going to argue that what I suggested in this PR is not wrong. Still, I would appreciate some more explanation where is the line.

I suggest to fix this in the Lexeme codebase, and not fiddle with this component to much.

That might have indeed be faster/easier in terms of getting code merged, but fixing the issue upstream felt more right (tm) thing to do.

Also notice this particular patch against master will be of no use for Wikibase, as the master branch of this component does not support PHP 5 any more

Thanks, I have been aware of that. Simply doing changes first to master and then backporting it to 2.x branch seemed the most appropriate way to proceed to me.

[thiemowmde]

Here is a quick protocol of what @manicki and I talked about today:

• I think this patch is fine and can be merged.
• The backport plan to a 2.x branch sounds good.
• I'm willing to merge all this. Please poke me again when you think the necessary patches are ready.
• The only technical concern I have is: Even if this patch looks like a bugfix, it might come with a change in behavior. This might affect projects that rely on this component, most notably SemanticMediaWiki. Please run all SemanticMediaWiki tests with this patch applied and make sure they are not affected. If this is given, I think we can release this as a bugfix release.

[JeroenDeDauw]

Looks good!

we've mentioned that further extension would be to allow to provide a function/object that would allow custom object comparison if something more sophisticated than PHP's native object non-strict comparison (i.e. all attribute and their values comparison) is needed.

If you start work on such a thing, please beware other code in this component already does such stuff. Check ValueComparer and implementations, and the usages of the interface.

projects that rely on this component, most notably SemanticMediaWiki

Unless I am really confoose, SMW does not use Diff

[JeroenDeDauw]

I've updated the release notes to include this change. I'll make a release on the master branch once I am back in Berlin, assuming you don't need that ASAP anyway. Go ahead and backport and tag a bugfix release on the PHP 5 branch as you see fit, +2 from me.

[thiemowmde]

SMW does not use Diff

Then I really don't know what the point of #74 was. Who else uses Diff?

[JeroenDeDauw]

To answer that you could look at the stars on GitHub, the forks, the dependents on packagist, the daily installs of the PHP 7 only version, ...