r101352 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r101351‎ | r101352 | r101353 >
Date:11:51, 31 October 2011
Author:hashar
Status:resolved (Comments)
Tags:
Comment:
unconditionally register the test modules

Somehow, you really need to have the modules registered globally.
Need to sort it out with Roan :-D

follow up r101346
Modified paths:
  • /branches/JSTesting/includes/resourceloader/ResourceLoader.php (modified) (history)
  • /branches/JSTesting/includes/specials/SpecialJavaScriptTest.php (modified) (history)

Diff [purge]

Index: branches/JSTesting/includes/resourceloader/ResourceLoader.php
——@@ -200,6 +200,11 @@
201201 // Register core modules
202202 $this->register( include( "$IP/resources/Resources.php" ) );
203203
 204+ // Register test Modules
 205+ // TODO: figures out how to register them conditionally and not
 206+ // for every page requests.
 207+ $this->registerTestModules();
 208+
204209 // Register extension modules
205210 wfRunHooks( 'ResourceLoaderRegisterModules', array( &$this ) );
206211 $this->register( $wgResourceModules );
Index: branches/JSTesting/includes/specials/SpecialJavaScriptTest.php
——@@ -29,8 +29,6 @@
3030 return;
3131 }
3232
33 - $out->getResourceLoader()->registerTestModules();
34 -
3533 $out->addModules( 'mediawiki.special.javaScriptTest' );
3634
3735 // Determine framework

Follow-up revisions

RevisionCommit summaryAuthorDate
r101353remove exception added by r101346...hashar11:56, 31 October 2011
r101383[JSTesting] Restore loading principle....krinkle18:56, 31 October 2011
r101384[JSTesting] Match the other wgEnableJavaScriptTest checks (just in case)....krinkle18:57, 31 October 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r101346now explicitly need to register test modules....hashar10:15, 31 October 2011

Comments

#Comment by Krinkle (talk | contribs)   18:57, 31 October 2011

This is exactly why I had the code in ResourceLoader.php instead of in the SpecialPage (which is how it was in an earlier version of the patch).

ResourceLoader doesn't can't and should not support loading of modules based on the current page because modules must be registered globally in order to allow javascript to load them on-demand and from a cached server with a common page independent load.php url.

Even if it would always be registered unconditionally, the amount of stuff added to the startup module isn't a huge deal, since that's cached anyway. To reduce this load it was previously only registered if wgEnableJavaScriptTest is true, which doesn't affect regular wikis (only wikis intended to be used as test runners, at which most requests will likely be to SpecialJavaScriptTest) - production wikis should never enable this mode due to the potential destructive nature of unit tests.

I've restored the situation to how it was with commit r101383, I kept it in a separate function like you did.

#Comment by Hashar (talk | contribs)   20:13, 31 October 2011

Thanks for the feedback! That is what Roan explained me this morning.

Somehow I wanted to avoid the global registration pollution for something that is not used usually. Looks like it does not involve any performance hit and I have lost my time :-(

#Comment by Catrope (talk | contribs)   08:58, 1 November 2011

Preventing test module registrations from polluting the startup module is possible, if we put them in a second startup-like module. This would involve a little bit of magic inside RL but overall wouldn't be that hard to do.

Status & tagging log