Page MenuHomePhabricator

Assert.php: Bad value for parameter $oldContent: must be a TextContent|null
Open, Needs TriagePublic

Description

Error

Request ID: XV-edQpAIC0AAFFJeEMAAABM
Request URL: www.wikidata.org//w/index.php?title=Q33232729&curid=34683193&diff=11&oldid=772301402

message
[XV-edQpAIC0AAFFJeEMAAABM] /w/index.php?title=Q33232729&curid=34683193&diff=11&oldid=772301402   Wikimedia\Assert\ParameterTypeException from line 89 of /srv/mediawiki/php-1.34.0-wmf.19/vendor/wikimedia/assert/src/Assert.php: Bad value for parameter $oldContent: must be a TextContent|null
trace
#0 /srv/mediawiki/php-1.34.0-wmf.19/includes/diff/SlotDiffRenderer.php(86): Wikimedia\Assert\Assert::parameterType(string, Wikibase\ItemContent, string)
#1 /srv/mediawiki/php-1.34.0-wmf.19/includes/diff/TextSlotDiffRenderer.php(103): SlotDiffRenderer->normalizeContents(Wikibase\ItemContent, WikitextContent, string)
#2 /srv/mediawiki/php-1.34.0-wmf.19/includes/diff/DifferenceEngine.php(1069): TextSlotDiffRenderer->getDiff(Wikibase\ItemContent, WikitextContent)
#3 /srv/mediawiki/php-1.34.0-wmf.19/includes/diff/DifferenceEngine.php(987): DifferenceEngine->getDiffBody()
#4 /srv/mediawiki/php-1.34.0-wmf.19/includes/diff/DifferenceEngine.php(949): DifferenceEngine->getDiff(string, string, string)
#5 /srv/mediawiki/php-1.34.0-wmf.19/includes/diff/DifferenceEngine.php(717): DifferenceEngine->showDiff(string, string, string)
#6 /srv/mediawiki/php-1.34.0-wmf.19/includes/page/Article.php(934): DifferenceEngine->showDiffPage(boolean)
#7 /srv/mediawiki/php-1.34.0-wmf.19/includes/page/Article.php(618): Article->showDiffPage()
#8 /srv/mediawiki/php-1.34.0-wmf.19/extensions/Wikibase/repo/includes/Actions/ViewEntityAction.php(79): Article->view()
#9 /srv/mediawiki/php-1.34.0-wmf.19/extensions/Wikibase/repo/includes/Actions/ViewEntityAction.php(54): Wikibase\ViewEntityAction->showEntityPage()
#10 /srv/mediawiki/php-1.34.0-wmf.19/includes/MediaWiki.php(507): Wikibase\ViewEntityAction->show()
#11 /srv/mediawiki/php-1.34.0-wmf.19/includes/MediaWiki.php(302): MediaWiki->performAction(Article, Title)
#12 /srv/mediawiki/php-1.34.0-wmf.19/includes/MediaWiki.php(892): MediaWiki->performRequest()
#13 /srv/mediawiki/php-1.34.0-wmf.19/includes/MediaWiki.php(523): MediaWiki->main()
#14 /srv/mediawiki/php-1.34.0-wmf.19/index.php(42): MediaWiki->run()
#15 /srv/mediawiki/w/index.php(3): include(string)
#16 {main}
Impact

11 hits in the last hour, 11 hits in the last 7 days, 36 hits in the last 30 days.

Notes

Not blocking train since it's not new. Only happens at wikidatawiki.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 23 2019, 1:36 PM
TextSlotDiffRenderer->getDiff(Wikibase\ItemContent, WikitextContent)

Yeah, diffing item and wikitext contents isn’t going to work… but apparently that’s what this URL is asking for?

index.php?title=Q33232729&curid=34683193&diff=11&oldid=772301402

Revision 11 belongs to a wikitext page, revision 772301402 to an item page. I don’t think there’s any reasonable interpretation of this URL (and I wonder where this request is coming from), we just need to handle it better.

On the other hand, other content models sometimes seem to support this – this diff, for example, is between a wikitext page and a JSON page. (It also works if you swap the compared revisions, for what it’s worth.)

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:05 PM
Tgr added a comment.Oct 8 2019, 10:31 AM

The diff logic leaves it to the SlotDiffRenderer implementation to handle incompatible types. Sorry about that, it's a bit shoddy. I don't remember if there was some specific reason for not turning it into a user error, or it was merely rushed.

From a distance, the simple solution would be to create a UserPageError subclass describing the situation and throw that instead of the assertion error. All SlotDiffRenderer subclasses would still have to implement the check but with a helper method that's just a one-liner and we don't want to preclude a differ's ability to provide special handling for different content types (even if there's another wart there, that depending on which content is on the left and which on the right side, you'll probably end up calling a different slot diff renderer implementation).

TextSlotDiffRenderer can compare any two TextContent subclasses. I don't think there's a real example today of a SlotDiffRenderer doing anything with a "foreign" content type, but there are use cases where it seems reasonable, especially when the page can be converted between the various types (e.g. wikitext <-> Flow <-> LiquidThreads).

Tgr added a comment.Oct 8 2019, 10:34 AM

From a distance, the simple solution would be to create a UserPageError subclass describing the situation and throw that instead of the assertion error.

One thing to keep in mind though is that the slot diff renderers are low-level code which is invoked in all kinds of contexts, not just diff pages. Sometimes in contexts where replacing the diff with an error message would be much better than killing the request altogether.

Still, it seems strictly less bad than the current behavior.

Special:ComparePages makes it better https://www.wikidata.org/wiki/Special:ComparePages?page1=&rev1=11&page2=&rev2=772301402&action=&diffonly=&unhide=

Can not diff an entity with a non-entity content.

wikibase-non-entity-diff

But not the other way round: https://www.wikidata.org/wiki/Special:ComparePages?page1=&rev1=772301402&page2=&rev2=11&action=&diffonly=&unhide=

[XaGZeQpAIC8AAHHLlKwAAAAI] 2019-10-12 09:14:33: Fatal „Wikimedia\Assert\ParameterTypeException“

So the question seems which diffengine is instanced for the compare and what for checks it does