Page MenuHomePhabricator

Make EntityId (ItemId etc.) not equal to its serialization by changing its string representation
Open, Needs TriagePublic

Description

Currently, a SerializableEntityId’s string representation is its serialization, which means it’s equal (==) to its serialization string.

>>> ( new \Wikibase\DataModel\Entity\ItemId( 'Q1' ) ) == "Q1"
=> true

This means that tests involving entity IDs are less strict than they should be: assertEquals( new ItemId( 'Q1' ), $actual ) will pass if $actual is a string, even if the test meant to expect an ItemId. This can hide real bugs: see T293104 for an example.

The comment of the __toString() method mentions that “[t]his format is allowed to change and should therefore not be relied upon to be stable”, but in practice we have plenty of tests (and, presumably, non-test code) relying on this behavior: see the results of this WIP change (some 40 to 80 failures, depending on the CI job). I suggest that we fix all these places – it’s not an insurmountable number – and then change __toString() to return something that’s still human-readable, but not exactly the serialization (e.g. adding the class name).

Event Timeline

Lucas_Werkmeister_WMDE renamed this task from Make EntityId not equal to its serialization to Make EntityId (ItemId etc.) not equal to its serialization by changing its string representation.Mar 23 2023, 11:41 AM

Change #1099210 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/ArticlePlaceholder@master] Use TermSearchResult::getEntityIdSerialization()

https://gerrit.wikimedia.org/r/1099210

Change #1099210 merged by jenkins-bot:

[mediawiki/extensions/ArticlePlaceholder@master] Use TermSearchResult::getEntityIdSerialization()

https://gerrit.wikimedia.org/r/1099210

The comment of the __toString() method mentions that “[t]his format is allowed to change and should therefore not be relied upon to be stable”

This was actually outdated information – two months before I wrote this, @Jakob_WMDE had already changed the documentation to “TODO: Consider removing this method in favor of just always calling getSerialization().” Great minds think alike and all that ;)

(I don’t know how I ended up with the old doc comment; this was shortly after we moved the data-model library into Wikibase.git, so there was no reason for me to see an outdated version in a vendor directory AFAICT. Anyway, not important.)