Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ListPatcher: use non-strict comparison when removing objects #92

Merged
merged 1 commit into from Apr 10, 2018

Conversation

@manicki
Copy link
Member

commented Apr 6, 2018

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 manicki requested a review from JeroenDeDauw Apr 6, 2018
@manicki

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

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

@thiemowmde

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

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.
private function newObject( $value ) : stdClass {
$object = new stdClass();
$object->element = $value;
return $object;

This comment has been minimized.

Copy link
@thiemowmde

thiemowmde Apr 10, 2018

Contributor

return (object)[ 'element' => $value ] is a clever 1-liner to do the same. ;-)

This comment has been minimized.

Copy link
@JeroenDeDauw

JeroenDeDauw Apr 10, 2018

Contributor

This way you also avoid the assignment to a field that was not explicitly defined

$this->newObject( 'bar' ),
);
$diff = new Diff( array(
new DiffOpRemove( $this->newObject( 'foo') ),

This comment has been minimized.

Copy link
@JeroenDeDauw

JeroenDeDauw Apr 10, 2018

Contributor

Spaceeeeeeeeeeeeee

This comment has been minimized.

Copy link
@manicki

manicki Apr 11, 2018

Author Member

fixed on master skipping the PR. PHPCS wake up!

@JeroenDeDauw

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

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 JeroenDeDauw merged commit c7ac65b into master Apr 10, 2018
3 checks passed
3 checks passed
Scrutinizer 2 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@JeroenDeDauw JeroenDeDauw deleted the listpatcher-objects branch Apr 10, 2018
return $argLists;
}
private function newObject( $value ) : stdClass {

This comment has been minimized.

Copy link
@JeroenDeDauw

JeroenDeDauw Apr 10, 2018

Contributor

First time I see stfClass as return type hint. Might also be the last, since as of PHP 7.2 object is probably the better one

This comment has been minimized.

Copy link
@manicki

manicki Apr 11, 2018

Author Member

stdclass as a return type hint indeed seems awkward. This is what using ancient language versions does with you

@JeroenDeDauw

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

SMW does not use Diff

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

@JeroenDeDauw

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.