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

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

In T231084#5554961, @Tgr wrote:

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

In T231084#5444984, @Lucas_Werkmeister_WMDE wrote:

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

It's because JsonContent extends TextContent thus it passes the check while EntityContent doesn't (we can also make EntityContent extend it but it seems a little weird to me)

In T231084#5569517, @Umherirrender wrote:

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

The traceback for this fatal doesn't hit any part of Wikibase code, making it hard to fix:

Backtrace:

#0 /var/lib/mediawiki2/includes/diff/SlotDiffRenderer.php(86): Wikimedia\Assert\Assert::parameterType(string, Wikibase\ItemContent, string)
#1 /var/lib/mediawiki2/includes/diff/TextSlotDiffRenderer.php(119): SlotDiffRenderer->normalizeContents(Wikibase\ItemContent, WikitextContent, string)
#2 /var/lib/mediawiki2/includes/diff/DifferenceEngine.php(1137): TextSlotDiffRenderer->getDiff(Wikibase\ItemContent, WikitextContent)
#3 /var/lib/mediawiki2/includes/diff/DifferenceEngine.php(1055): DifferenceEngine->getDiffBody()
#4 /var/lib/mediawiki2/includes/diff/DifferenceEngine.php(1017): DifferenceEngine->getDiff(string, string, string)
#5 /var/lib/mediawiki2/includes/diff/DifferenceEngine.php(781): DifferenceEngine->showDiff(string, string, string)
#6 /var/lib/mediawiki2/includes/specials/SpecialComparePages.php(128): DifferenceEngine->showDiffPage(boolean)
#7 /var/lib/mediawiki2/includes/htmlform/HTMLForm.php(694): SpecialComparePages::showDiff(array, OOUIHTMLForm)
#8 /var/lib/mediawiki2/includes/specials/SpecialComparePages.php(109): HTMLForm->trySubmit()
#9 /var/lib/mediawiki2/includes/specialpage/SpecialPage.php(575): SpecialComparePages->execute(NULL)
#10 /var/lib/mediawiki2/includes/specialpage/SpecialPageFactory.php(611): SpecialPage->run(NULL)
#11 /var/lib/mediawiki2/includes/MediaWiki.php(298): MediaWiki\Special\SpecialPageFactory->executePath(Title, RequestContext)
#12 /var/lib/mediawiki2/includes/MediaWiki.php(967): MediaWiki->performRequest()
#13 /var/lib/mediawiki2/includes/MediaWiki.php(530): MediaWiki->main()
#14 /var/lib/mediawiki2/index.php(46): MediaWiki->run()
#15 {main}

The main one is not super hard to fix though. I can put a try catch in ViewEntityAction::showEntityPage():

		try {
			$this->page->view();
		} catch ( ParameterTypeException $exception ) {
			if ( !$this->isDiff() === true ) {
				throw $exception;
			}
		}

Not sure this is the best way to move forward. One other rather easy way is to have the try catch in DifferenceEngine::showDiffPage.

(See also T202763: Update extensions which customize content diff rendering.)

DifferenceEngine calls the renderer of the right-hand item (so if you compare wikibase item to wikitext, it will call TextSlotDiffRenderer, if you compare wikitext to wikibase item, it will call EntityContentDiffView). That's not great, extensions should be able to provide wikitext diffing logic without having to override the wikitext diff renderer class. I'm not sure what would be the sane way to achieve that, though. If you compare A and B and the diff renderer for A throws an exception, catch and call the renderer for B? That feels very dirty; even if we replace exception catching with some kind of capability detection mechanism, it seems fragile.

Another option would be something like:

				$de = $contentHandler->createDifferenceEngine( $form->getContext(),
					$rev1,
					$rev2,
					null, // rcid
					( $data['Action'] == 'purge' ),
					( $data['Unhide'] == '1' )
				);
				if(!$de->canDiffRevisions();){
                                        // Throw special page error here
                                }
				$de->showDiffPage( true );

With canDiffRevisions being implemented in DifferenceEngine which would see if the 2 contents say they can be diffed with each other?
I mean, when would it ever make sense to even try and diff wikitext and a wikibase entity?

In T231084#5839574, @Addshore wrote:

With canDiffRevisions being implemented in DifferenceEngine which would see if the 2 contents say they can be diffed with each other?

How would DifferenceEngine know that? It has the same dependency on page type as the slots.

I mean, when would it ever make sense to even try and diff wikitext and a wikibase entity?

It probably doesn't. It would still be nice if Wikibase could make that call.

Anyway the immediate solution is to just throw a UserPageError instead of the assertion error, and have it tell the user that incompatible revisions are being compared (and maybe tell the content models).

In the longer term, what if you compare a (wikitext, foo) MCR record with a (wikibase, foo) one? Does it still make sense to at least the foo part diffed? If so, DifferenceEngine would probably need its own error handling, which turns the exception into part of the diff HTML.

Change 619037 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] Convert different content model in DiffEngine before create diff

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

In T231084#6781703, @Krinkle wrote:

Inability to patrol or review certain Wikidata diffs, seems high priority.

@Ladsgroup @Addshore I was going to re-tag but I see it was intentionally untagged, is this issue tracked elsewhere or perhaps not what it seems? It seems high priority, but perhaps it's an edge case.

This is not wikidata-related error. It just happens to happen in Wikidata because we have lots of revision that have a different content type than wikitext (and diffing between these two is not properly caught). As I said before, the traceback for this fatal doesn't hit any part of Wikibase code. It also happens in other forms in other extensions as well. T265524: Cannot diff content types other than MassMessageListContent should be merged as duplicate of this.

TLDR: It's core.

In T265524#6838060, @dancy wrote:

Errors like this are coming in at a steady rate starting around 8AM UTC today. 344 in the last 30 minutes.
Here's a fresh stack trace:

Stack Trace

from /srv/mediawiki/php-1.36.0-wmf.30/extensions/MassMessage/includes/Content/MassMessageListDiffEngine.php(29)
#0 /srv/mediawiki/php-1.36.0-wmf.30/includes/diff/DifferenceEngineSlotDiffRenderer.php(57): MediaWiki\MassMessage\Content\MassMessageListDiffEngine->generateContentDiffBody(WikitextContent, MediaWiki\MassMessage\Content\MassMessageListContent)
#1 /srv/mediawiki/php-1.36.0-wmf.30/includes/diff/DifferenceEngine.php(1263): DifferenceEngineSlotDiffRenderer->getDiff(WikitextContent, MediaWiki\MassMessage\Content\MassMessageListContent)
#2 /srv/mediawiki/php-1.36.0-wmf.30/includes/api/ApiComparePages.php(182): DifferenceEngine->getDiffBody()
#3 /srv/mediawiki/php-1.36.0-wmf.30/includes/api/ApiMain.php(1612): ApiComparePages->execute()
#4 /srv/mediawiki/php-1.36.0-wmf.30/includes/api/ApiMain.php(592): ApiMain->executeAction()
#5 /srv/mediawiki/php-1.36.0-wmf.30/includes/api/ApiMain.php(563): ApiMain->executeActionWithErrorHandling()
#6 /srv/mediawiki/php-1.36.0-wmf.30/api.php(90): ApiMain->execute()
#7 /srv/mediawiki/php-1.36.0-wmf.30/api.php(45): wfApiMain()
#8 /srv/mediawiki/w/api.php(3): require(string)
#9 {main}

URL: https://zh.wikipedia.org/w/api.php?action=compare&fromtitle=Wikipedia%3AWikipediaCatNews/SList&format=json&fromrev=64329173&torev=64342572

Change 619037 abandoned by Umherirrender:
[mediawiki/core@master] DifferenceEngine: Convert different content models before creating diff

Reason:
Unblock the task and allower another developer to provide a better fix

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

Chatted with @Addshore, and here is what I know:

• This error is not believed to be caused by Wikibase. There is not a problem where a page has a history that involves incompatible content models. In fact, MediaWiki generally doesn't allow you to change content models of a page except from/to content modelse that have a common base type or otherwise have a way to be transformed, and thus compared.
• The problem is believed to be caused (only) by accidental providing of two incompatible page titles (Special:ComparePages), or two incompatible revision ids (ApiCompare, or diff URL parameter editing). There is no productive outcome expected here. Rendering an error seems fine here from a product perspective.
• That that leaves us with is making sure the application doesn't crash, and thus doesn't eat into our operational error budget.

Two ideas come to mind:

1. Informs the user what happened - namely that the specified pages/revisions have an incompatible content model. This would be a normal user-error localisable message.
2. Fall back to rendering both revisions in a plain text <pre> using their own serialised forms. This is similar to what we do when you view the first revision of a page and there is no previous revision to show. This might be more useful and means if the user really wanted to compare the two, they won't have to do something hacky like "edit old revision" of both revisions in order to see their source side by side.

Also one other note: It might actually be possible for Wikibase ItemContent to become "compatible" with core's TextContent. Core's CSS/JS/JSON content models are also compatible with that. If that was done, then this error would no longer happen on wikidata.org, but might still happen on other wikis with other incompatible content models, so the general issue still stands as well.

Yes. T265524: Cannot diff content types other than MassMessageListContent is an example that doesn't involve Wikibase at all.

@marcella The maintainers states that the "Page Diffs" feature in core is owned by the "Contributors Team". This name has not been used in some time as an actual team (as opposed to superteam), but I do not know which team this component ended up with after the various re-orgs (e.g. editing, growth, parsing, etc.) Do you know where this should go?

I don't know if it's interesting, but I also saw this happen on notwikilambda when playing around with it a bit.

[614fa6e8509a8e1eda33cd92] /w/index.php?title=ZObject:Z10019&curid=986&diff=2297&oldid=2264 Wikimedia\Assert\ParameterTypeException: Bad value for parameter $oldContent: must be a TextContent|null

Backtrace:

from /data/project/notwikilambda/public_html/w/vendor/wikimedia/assert/src/Assert.php(105)
#0 /data/project/notwikilambda/public_html/w/includes/diff/SlotDiffRenderer.php(89): Wikimedia\Assert\Assert::parameterType(array, MediaWiki\Extension\WikiLambda\ZObjectContent, string)
#1 /data/project/notwikilambda/public_html/w/includes/diff/TextSlotDiffRenderer.php(125): SlotDiffRenderer->normalizeContents(MediaWiki\Extension\WikiLambda\ZObjectContent, MediaWiki\Extension\WikiLambda\ZObjectContent, string)
#2 /data/project/notwikilambda/public_html/w/includes/diff/DifferenceEngine.php(1216): TextSlotDiffRenderer->getDiff(MediaWiki\Extension\WikiLambda\ZObjectContent, MediaWiki\Extension\WikiLambda\ZObjectContent)
#3 /data/project/notwikilambda/public_html/w/includes/diff/DifferenceEngine.php(1124): DifferenceEngine->getDiffBody()
#4 /data/project/notwikilambda/public_html/w/includes/diff/DifferenceEngine.php(1086): DifferenceEngine->getDiff(string, string, string)
#5 /data/project/notwikilambda/public_html/w/includes/diff/DifferenceEngine.php(852): DifferenceEngine->showDiff(string, string, string)
#6 /data/project/notwikilambda/public_html/w/includes/page/Article.php(930): DifferenceEngine->showDiffPage(boolean)
#7 /data/project/notwikilambda/public_html/w/includes/page/Article.php(511): Article->showDiffPage()
#8 /data/project/notwikilambda/public_html/w/includes/actions/ViewAction.php(74): Article->view()
#9 /data/project/notwikilambda/public_html/w/includes/MediaWiki.php(538): ViewAction->show()
#10 /data/project/notwikilambda/public_html/w/includes/MediaWiki.php(320): MediaWiki->performAction(Article, Title)
#11 /data/project/notwikilambda/public_html/w/includes/MediaWiki.php(925): MediaWiki->performRequest()
#12 /data/project/notwikilambda/public_html/w/includes/MediaWiki.php(559): MediaWiki->main()
#13 /data/project/notwikilambda/public_html/w/index.php(53): MediaWiki->run()
#14 /data/project/notwikilambda/public_html/w/index.php(46): wfIndexMain()
#15 {main}

In T231084#7152005, @Krinkle wrote:

In fact, MediaWiki generally doesn't allow you to change content models of a page except from/to content modelse that have a common base type or otherwise have a way to be transformed, and thus compared.

You can do creative things with deletion/undeletion and page move, but in practice, yeah, it's unlikely to happen.

Anyway, it is certainly the case that

There is no productive outcome expected here. Rendering an error seems fine here from a product perspective.

Probably the low-effort fix is to throw an ErrorPageError instead of an assertion error.

Seems like this got fixed in core – the request URL now produces:

I have removed a log filter from Kibana MediaWiki New Errors dashboard which pointed back to this task. There is no hit over the last four weeks.

The filter was:

{
  "query": {
    "regexp": {
      "labels.normalized_message": {
        "value": "Bad value for parameter .oldContent: must be a.+"
      }
    }
  }
}

There was an another filter mentioning this task which I have removed as well:

{
  "query": {
    "match_phrase": {
      "error.message": "Cannot diff content types other than MassMessageListContent"
    }
  }
}