Change 617163 had a related patch set uploaded (by MSantos; owner: MSantos):
[mediawiki/extensions/JsonConfig@master] Cache loadLocal shouldn't raise Exception

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

I believe this was triggered by this change: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/a67cad6d0fc5b6a2aab08e5dd9b0391202ea3e9b

JsonConfig doesn't (yet) register its content handlers in extension.json, but rather handles them in a hook handler for ContentHandlerForModelID. As such, its content models aren't known to the ContentHandlerFactory. Previously, RevisionStore::loadSlotContent() would call ContentHandlerFactory::getContentHandler() without first checking if the content model was known, ContentHandlerFactory would first attempt to look up the handler from the known specs and fail, and it would then run the ContentHandlerForModelID hook, where JsonConfig would return the correct handler. Now, the new check short-circuits the hook used by ContentHandlerFactory as a fallback and instead returns a FallbackContent object for unregistered content models; and the error here is triggered when JsonConfig encounters a FallbackContent object, which isn't an instance of JCContent.

I think there's probably some work to be done to harmonize the new check with the behavior of ContentHandlerFactory (and maybe this wasn't the only extension relying on the fallback behavior?) but I think the easiest path forward to unbreak this is to update JsonConfig to register its content handlers in extension.json.

See also: https://logstash.wikimedia.org/goto/444b53f9628148901fafe2ff06450658

Change 617255 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/JsonConfig@master] Register content handlers in extension.json

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

Change 617257 had a related patch set uploaded (by Mholloway; owner: Michael Holloway):
[mediawiki/extensions/Dashiki@master] Register content handler in extension.json

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

I believe the above patches also fix T159226: JsonConfig sets config models that aren't shown/selectable on Special:ChangeContentModel.

And T155144: unable to save json datasets using api.

The fundamental issue seems to be T155139: 'ContentHandlerForModelID' hook allows creating of handlers that aren't registered; based on discussion on that ticket, another possible approach to fixing this would be to have JsonConfig (and Dashiki) implement a getContentModels hook handler (that is, if there's some strong reason not to register their content handlers directly, as the discussion seems to imply there is).

In T259126#6346542, @Mholloway wrote:

I believe this was triggered by this change: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/a67cad6d0fc5b6a2aab08e5dd9b0391202ea3e9b

JsonConfig doesn't (yet) register its content handlers in extension.json, but rather handles them in a hook handler for ContentHandlerForModelID. As such, its content models aren't known to the ContentHandlerFactory. Previously, RevisionStore::loadSlotContent() would call ContentHandlerFactory::getContentHandler() without first checking if the content model was known, ContentHandlerFactory would first attempt to look up the handler from the known specs and fail, and it would then run the ContentHandlerForModelID hook, where JsonConfig would return the correct handler. Now, the new check short-circuits the hook used by ContentHandlerFactory as a fallback and instead returns a FallbackContent object for unregistered content models; and the error here is triggered when JsonConfig encounters a FallbackContent object, which isn't an instance of JCContent.

I think there's probably some work to be done to harmonize the new check with the behavior of ContentHandlerFactory (and maybe this wasn't the only extension relying on the fallback behavior?) but I think the easiest path forward to unbreak this is to update JsonConfig to register its content handlers in extension.json.

Wouldn't it make sense to fix the ContentHandlerFactory / ContentHandlerForModelID hook so it works as it's supposed to? That seems like a breaking change that shouldn't have happened.

Maybe JsonConfig should implement the GetContentModels hook? That would ensure ContentHandlerFactory::isDefinedModel() returns true.

Change 617273 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/extensions/JsonConfig@master] Implement GetContentModels hook

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

@Mholloway how does ^ look? I didn't test it. I think it's smaller and fits the expectations by ContentHandlerFactory (though I do think the core change might need to be re-evaluated).

In T259126#6346962, @Legoktm wrote:

Maybe JsonConfig should implement the GetContentModels hook? That would ensure ContentHandlerFactory::isDefinedModel() returns true.

Thanks, this is probably a better way to go, actually. I'll kick the tires and hit +2 if everything looks good.

In T259126#6346994, @Legoktm wrote:

(though I do think the core change might need to be re-evaluated)

Yeah, I went for the extension fix(es) in the first instance because I wasn't sure if this was part of a broader change in how content handlers are to be registered. I agree that some of these edge cases could use a bit more consideration. IMHO the ContentHandlerForModelID hook (usage) actually looks like a good candidate for deprecation and eventual removal, but that's going a bit out of scope for now, I think.

Change 617257 abandoned by Mholloway:
[mediawiki/extensions/Dashiki@master] Register content handler in extension.json

Reason:
Superseded by Ida2d2aa1dbd6e9e2041c1dbb351788a139eef0bb

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

Change 617255 abandoned by Mholloway:
[mediawiki/extensions/JsonConfig@master] Register content handlers in extension.json

Reason:
Superseded by Ida2d2aa1dbd6e9e2041c1dbb351788a139eef0bb

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

Change 617306 had a related patch set uploaded (by Mholloway; owner: Legoktm):
[mediawiki/extensions/JsonConfig@wmf/1.36.0-wmf.2] Implement GetContentModels hook

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

Change 617273 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@master] Implement GetContentModels hook

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

Change 617306 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@wmf/1.36.0-wmf.2] Implement GetContentModels hook

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

Mentioned in SAL (#wikimedia-operations) [2020-07-30T01:07:19Z] <mholloway-shell@deploy1001> Synchronized php-1.36.0-wmf.2/extensions/JsonConfig: Backport: Implement GetContentModels hook (T259126) (duration: 01m 07s)

Promoted back to group1, out of train window. Wanted to do this early so that if everything seems OK for a few hours, we (me or Brennen) can promote to group2 optimistic-opportunistically. If that fails, we roll back to group1 and try again on Monday if any problems found have been fixed.

This deployment's sound track was Queen's "We will rock you".

Change 617163 abandoned by MSantos:
[mediawiki/extensions/JsonConfig@master] Cache loadLocal unsupported model logging improvement

Reason:

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