Page MenuHomePhabricator

'ContentHandlerForModelID' hook allows creating of handlers that aren't registered
Closed, ResolvedPublic

Description

Content handlers are supposed to be registered in $wgContentHandlers, so they can be enumerated by ContentHandler::getContentModels() (for the API and for other purposes). But the 'ContentHandlerForModelID' hook allows for creating handlers in ContentHandler::getForModelID() that are not registered.

The end result is a handler that mostly exists, but can't be used with the API or Special:ChangeContentModel, doesn't get tested by ContentHandlerSanityTest, and doesn't get its getFieldsForSearchIndex() method called by SearchEngine.

One possible solution would be to only call the hook for entries in $wgContentHandlers where the value is null, to allow the handlers to be created on demand with dependency injection while still being properly registered.

Extensions (mis)using this hook appear to be MediaWiki-extensions-WikibaseRepository and MediaWiki-extensions-JsonConfig.

Related Objects

StatusAssignedTask
ResolvedWikidata-bugs
OpenNone
Resolvedaude
ResolvedSmalyshev
Resolvedaude
ResolvedNone
InvalidNone
ResolvedSmalyshev
ResolvedLydia_Pintscher
DuplicateSmalyshev
DuplicateNone
DeclinedNone
DeclinedNone
Resolveddaniel
ResolvedLydia_Pintscher
OpenNone
DeclinedNone
ResolvedSmalyshev
ResolvedSmalyshev
DeclinedNone
ResolvedSmalyshev
ResolvedSmalyshev
ResolvedSmalyshev
ResolvedSmalyshev
Resolveddcausse
Resolveddcausse
ResolvedSmalyshev
ResolvedSmalyshev
ResolvedSmalyshev
ResolvedSmalyshev
Resolveddcausse

Event Timeline

Anomie created this task.Jan 11 2017, 11:15 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 11 2017, 11:15 PM

One thing to consider is that we may support legacy content models for old revisions that we do want to allow for new revisions.

In any case, an alternative to putting null entries into $wgContentHandlers would be to add another hook, GetContentModels, which is called from ContentHandler::getContentModels(). This isn't very pretty either, but if an extension is implementing one hook, it can just as well implement another.

On a higher level, this problem is caused by the fact that extension.json cannot add closures to $wgContentHandlers. But it could add a static factory method. We should think about introducing a syntax into extension.json that lets us reference callbacks defined in some kind of wiring file. On the other hand - such a wiring file could just as well be a php lass with static callback methods, which could easily be used from extension.json.

TheDJ added a comment.Jan 13 2017, 1:21 AM

@daniel BTW: this seems to have some similarities to [[mw:Manual:Hooks/ResourceLoaderRegisterModules]], which allows extensions to do conditional registration of resourceloader modules.

I don't care too much if legacy ones do not show up in wgContentHandlers. In fact, I'd be completely happy if they didn't since then I don't need to keep search fields for them (even though empty mapped fields aren't super-expensive AFAIK). But it's be very nice if ContentHandler::getContentModels() showed currently used ones.

On a higher level, this problem is caused by the fact that extension.json cannot add closures to $wgContentHandlers. But it could add a static factory method.

Yes, Wikibase creates them by a static hook anyway, so it's just a question of who calls the hook and when.

daniel moved this task from Inbox to To Do on the User-Daniel board.Jan 30 2017, 11:26 AM
daniel triaged this task as High priority.

Change 335188 had a related patch set uploaded (by Smalyshev):
Add GetContentModels hook to allow extensions to enumerate dynamic content models.

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

Change 335189 had a related patch set uploaded (by Smalyshev):
Add GetContentModels hook for Wikibase

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

daniel moved this task from To Do to Doing on the User-Daniel board.Jan 31 2017, 6:01 PM

Change 335270 had a related patch set uploaded (by Daniel Kinzler):
Register ContentHandlers in $wgContentHandlers.

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

Change 335188 merged by jenkins-bot:
Add GetContentModels hook to allow extensions to enumerate dynamic content models.

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

Smalyshev closed this task as Resolved.Feb 8 2017, 7:55 PM
Smalyshev claimed this task.

I've split off Wikibase part to a new ticket. T155144 is for JsonConfig part of the same.

Change 335189 abandoned by Smalyshev:
Add GetContentModels hook for Wikibase

Reason:
Replaced by I73ab3f156427abe2

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

Change 335270 merged by jenkins-bot:
Register ContentHandlers in $wgContentHandlers.

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