Page MenuHomePhabricator

MF shouldn't change wgResourceModules inside ResourceLoaderRegisterModules hook
Closed, ResolvedPublic

Description

Summary: registerMobileLoggingSchemasModule should be refactored to use $resourceLoader->register

The registration hook is to register modules. Don't use it to add entries to $wgResourceModules.

This was never supported and happened to work since the hook was run in ResourceLoader.php exactly one line before the modules from wgResourceModules are registered.

This is changing in MediaWiki 1.26 and therefore this code must too.

Other extensions always used register() when inside the ResourceLoaderRegisterModules hook, that's what the hook is for.

To register modules in $wgResourceModules, either add them statically (in extension.json or the PHP entry point). Or, to add them dynamically, do so from a setup hook (e.g. wgExtensionFunctions).

The main culprit is MobileFrontendHooks::registerMobileLoggingSchemasModule which is re-used in multiple code pieces instead of explicitly doing it from one place.

Event Timeline

Krinkle created this task.Jun 16 2015, 9:29 PM
Krinkle raised the priority of this task from to High.
Krinkle updated the task description. (Show Details)
Krinkle added a subscriber: Krinkle.
Restricted Application added a project: Readers-Web-Backlog. · View Herald TranscriptJun 16 2015, 9:29 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Or, to add them dynamically, do so from a setup hook (e.g. wgExtensionFunctions).

Uh, no, don't do that either. Either make the definition static, or use the ResourceLoaderRegisterModules hook properly.

What would you propose we do here instead @Legoktm @Krinkle ?

The modules should be registered in ResourceLoaderRegisterModules?

Okay, sorry I misread. I'll remove Krinkle's alternative proposal from the description summary to make this clearer.

Jdlrobson updated the task description. (Show Details)Jun 22 2015, 9:53 PM

So... the idea/problem here was to only load the modules when EventLogging was installed, using EventLoggingRegisterSchemas hook, since they explode without it.

This hook doesn't seem to give access to ResourceLoader so this is not as simple as first suggested by @Legoktm and @Krinkle

So... the idea/problem here was to only load the modules when EventLogging was installed, using EventLoggingRegisterSchemas hook, since they explode without it.
This hook doesn't seem to give access to ResourceLoader so this is not as simple as first suggested by @Legoktm and @Krinkle

Umm, both Krinkle and I said to use the ResourceLoaderRegisterModules hook, not EventLoggingRegisterSchemas.

Jdlrobson added a subscriber: ori.Jul 7 2015, 1:13 AM

I understand that well. I'm saying the reason it currently loads via the EventLoggingRegisterSchemas is that the code currently breaks if loaded without event logging extension installed.

So I guess another question is what is the best way to check if EventLogging is installed before adding these modules. @ori told me to use this hook originally instead of using class_exists, but things may have changed since then.

ori added a comment.Jul 7 2015, 1:15 AM

You can actually just add them to $wgEventLoggingSchemas without worrying about whether EventLogging is installed or not.

$wgEventLoggingSchemas['MultimediaViewerNetworkPerformance'] = 7917896;

Yeh I know that bit - context: https://gerrit.wikimedia.org/r/#/c/120957/

my concern is I have a module that declares a dependency to one of those schemas e.g. dependencies => 'schema.MultimediaViewerNetworkPerformance'

Jdlrobson moved this task from Backlog to Bugs on the MobileFrontend board.Aug 4 2015, 6:36 PM
Jdlrobson added a subscriber: Jhernandez.

"This is changing in MediaWiki 1.26 and therefore this code must too."
We should probably dedicate some time @Jhernandez to get this done.

Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-54-28-Days-Later board.

Change 232304 had a related patch set uploaded (by Robmoen):
WIP: Don't change wgResourceLoaderModules inside hook

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

It's not merged. There's failures on gerrit! 💩

Change 232304 merged by jenkins-bot:
Register mobile.loggingSchemas with optional schema dependencies

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

phuedx closed this task as Resolved.Aug 27 2015, 11:00 AM
phuedx added a subscriber: phuedx.

This is a technical task.

Change 236758 had a related patch set uploaded (by Phuedx):
Register schemas using hooks not via globals

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

Change 236758 merged by jenkins-bot:
Register schemas using hooks not via globals

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