r72776 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r72775‎ | r72776 | r72777 >
Date:07:33, 11 September 2010
Author:tparscal
Status:resolved (Comments)
Tags:
Comment:
Fixed user scripts/styles and site scripts/styles - they were totally broken in r72772 - but now the refactoring is paying off.
Modified paths:
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/ResourceLoaderContext.php (modified) (history)
  • /trunk/phase3/includes/ResourceLoaderModule.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/ResourceLoaderContext.php
——@@ -86,7 +86,7 @@
8787 }
8888
8989 public function getUser() {
90 - return $this->skin;
 90+ return $this->user;
9191 }
9292
9393 public function getDebug() {
Index: trunk/phase3/includes/OutputPage.php
——@@ -2360,7 +2360,6 @@
23612361 );
23622362 }
23632363
2364 - // TODO: User Scripts should be included using the resource loader
23652364 // Add user JS if enabled
23662365 if( $this->isUserJsAllowed() && $wgUser->isLoggedIn() ) {
23672366 $action = $wgRequest->getVal( 'action', 'view' );
——@@ -2368,14 +2367,7 @@
23692368 # XXX: additional security check/prompt?
23702369 $this->addInlineScript( $wgRequest->getText( 'wpTextbox1' ) );
23712370 } else {
2372 - $userpage = $wgUser->getUserPage();
2373 - foreach( array( 'common', $sk->getSkinName() ) as $name ) {
2374 - $scriptpage = Title::makeTitleSafe( NS_USER, $userpage->getDBkey() . '/' . $name . '.js' );
2375 - if ( $scriptpage && $scriptpage->exists() && ( $scriptpage->getLength() > 0 ) ) {
2376 - $userjs = $scriptpage->getLocalURL( 'action=raw&ctype=' . $wgJsMimeType );
2377 - $this->addScriptFile( $userjs, $scriptpage->getLatestRevID() );
2378 - }
2379 - }
 2371+ $scripts .= self::makeResourceLoaderLink( $sk, 'user', 'scripts' );
23802372 }
23812373 }
23822374 $scripts .= "\n" . $this->mScripts;
Index: trunk/phase3/includes/ResourceLoaderModule.php
——@@ -703,29 +703,51 @@
704704
705705 abstract protected function getPages( ResourceLoaderContext $context );
706706
 707+ /* Protected Methods */
 708+
 709+ protected function getContent( $page, $ns ) {
 710+ if ( $ns === NS_MEDIAWIKI ) {
 711+ return wfMsgExt( $page, 'content' );
 712+ }
 713+ if ( $title = Title::newFromText( $page, $ns ) ) {
 714+ if ( $title->isValidCssJsSubpage() && $revision = Revision::newFromTitle( $title ) ) {
 715+ return $revision->getRawText();
 716+ }
 717+ }
 718+ return null;
 719+ }
 720+
707721 /* Methods */
708722
709723 public function getScript( ResourceLoaderContext $context ) {
 724+ global $wgCanonicalNamespaceNames;
 725+
710726 $scripts = '';
711727 foreach ( $this->getPages( $context ) as $page => $options ) {
712728 if ( $options['type'] === 'script' ) {
713 - $script = wfMsgExt( $page, 'content' );
714 - $scripts .= "/* MediaWiki:$page */\n" . ( !wfEmptyMsg( $page, $script ) ? $script : '' ) . "\n";
 729+ if ( $script = $this->getContent( $page, $options['ns'] ) ) {
 730+ $ns = $wgCanonicalNamespaceNames[$options['ns']];
 731+ $scripts .= "/*$ns:$page */\n$script\n";
 732+ }
715733 }
716734 }
717735 return $scripts;
718736 }
719737
720738 public function getStyles( ResourceLoaderContext $context ) {
 739+ global $wgCanonicalNamespaceNames;
 740+
721741 $styles = array();
722742 foreach ( $this->getPages( $context ) as $page => $options ) {
723743 if ( $options['type'] === 'style' ) {
724744 $media = isset( $options['media'] ) ? $options['media'] : 'all';
725 - $style = wfMsgExt( $page, 'content' );
726 - if ( !isset( $styles[$media] ) ) {
727 - $styles[$media] = '';
 745+ if ( $style = $this->getContent( $page, $options['ns'] ) ) {
 746+ if ( !isset( $styles[$media] ) ) {
 747+ $styles[$media] = '';
 748+ }
 749+ $ns = $wgCanonicalNamespaceNames[$options['ns']];
 750+ $styles[$media] .= "/* $ns:$page */\n$style\n";
728751 }
729 - $styles[$media] .= "/* MediaWiki:$page */\n" . ( !wfEmptyMsg( $page, $style ) ? $style : '' ) . "\n";
730752 }
731753 }
732754 return $styles;
——@@ -738,7 +760,7 @@
739761 }
740762 $titles = array();
741763 foreach ( $this->getPages( $context ) as $page => $options ) {
742 - $titles[] = Title::makeTitle( NS_MEDIAWIKI, $page );
 764+ $titles[] = Title::makeTitle( $options['ns'], $page );
743765 }
744766 // Do batch existence check
745767 // TODO: This would work better if page_touched were loaded by this as well
——@@ -765,14 +787,14 @@
766788 global $wgHandheldStyle;
767789
768790 $pages = array(
769 - 'Common.js' => array( 'type' => 'script' ),
770 - 'Common.css' => array( 'type' => 'style' ),
771 - ucfirst( $context->getSkin() ) . '.js' => array( 'type' => 'script' ),
772 - ucfirst( $context->getSkin() ) . '.css' => array( 'type' => 'style' ),
773 - 'Print.css' => array( 'type' => 'style', 'media' => 'print' ),
 791+ 'Common.js' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'script' ),
 792+ 'Common.css' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'style' ),
 793+ ucfirst( $context->getSkin() ) . '.js' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'script' ),
 794+ ucfirst( $context->getSkin() ) . '.css' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'style' ),
 795+ 'Print.css' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'style', 'media' => 'print' ),
774796 );
775797 if ( $wgHandheldStyle ) {
776 - $pages['Handheld.css'] = array( 'type' => 'style', 'media' => 'handheld' );
 798+ $pages['Handheld.css'] = array( 'ns' => NS_MEDIAWIKI, 'type' => 'style', 'media' => 'handheld' );
777799 }
778800 return $pages;
779801 }
——@@ -789,11 +811,12 @@
790812 global $wgAllowUserCss;
791813
792814 if ( $context->getUser() && $wgAllowUserCss ) {
793 - $user = User::newFromName( $context->getUser() );
794 - $userPage = $user->getUserPage()->getPrefixedText();
 815+ $username = $context->getUser();
795816 return array(
796 - "$userPage/common.css" => array( 'type' => 'style' ),
797 - "$userPage/" . $context->getSkin() . '.css' => array( 'type' => 'style' ),
 817+ "$username/common.js" => array( 'ns' => NS_USER, 'type' => 'script' ),
 818+ "$username/" . $context->getSkin() . '.js' => array( 'ns' => NS_USER, 'type' => 'script' ),
 819+ "$username/common.css" => array( 'ns' => NS_USER, 'type' => 'style' ),
 820+ "$username/" . $context->getSkin() . '.css' => array( 'ns' => NS_USER, 'type' => 'style' ),
798821 );
799822 }
800823 return array();
——@@ -816,8 +839,12 @@
817840 if ( isset( $this->modifiedTime[$hash] ) ) {
818841 return $this->modifiedTime[$hash];
819842 }
820 - $user = User::newFromName( $context->getUser() );
821 - return $this->modifiedTime[$hash] = $user->getTouched();
 843+ if ( $context->getUser() ) {
 844+ $user = User::newFromName( $context->getUser() );
 845+ return $this->modifiedTime[$hash] = $user->getTouched();
 846+ } else {
 847+ return 0;
 848+ }
822849 }
823850
824851 public function getStyles( ResourceLoaderContext $context ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r74691Improves on r72776 by adding documentation about the limitations of ResourceL...tparscal21:19, 12 October 2010
r81692Fixes for ResourceLoaderWikiModule r72776. No serious bugs found, do not merg...tstarling06:34, 8 February 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r72772Major refactoring of site and user CSS, creating ResourceLoaderUserModule and...tparscal03:26, 11 September 2010

Comments

#Comment by Catrope (talk | contribs)   14:39, 3 October 2010 
+			if ( $title->isValidCssJsSubpage() && $revision = Revision::newFromTitle( $title ) ) {

The documentation of ResourceLoaderWikiModule should mention that it can only be used on pages in the MediaWiki: namespace and user JS/CSS subpages, as opposed to arbitrary wiki pages. 

+					$ns = $wgCanonicalNamespaceNames[$options['ns']];

Use MWNamespace::getCanonicalName( $nsNumber ) 

+			'Common.js' => array( 'ns' => NS_MEDIAWIKI, 'type' => 'script' ),

Why are you using this totally crazy format? Just use full page names like 'MediaWiki:Common.js' or "User:$username/common.js" and let the Title class do its job (you're constructing titles from them in getContent() anyway). This also allows you to replace the awkward /* $ns:$page */ construct with $title->getPrefixedText() rendering my comment about $wgCanonicalNamespaceNames above moot.

#Comment by Trevor Parscal (WMF) (talk | contribs)   21:20, 12 October 2010

1. Yes, done in r74691 2. Awesome! also done in r74691 3. You aren't noticing that ResourceLoaderWikiModule::getContent needs to know which namespace it's in to either use wfMsgExt or Revision::getRawText - encoding it into the string would only move the complexity to string parsing.

#Comment by Catrope (talk | contribs)   16:36, 16 October 2010

That string parsing is in the Title class already. What you'd do is use full page names like I said, then construct Title objects from them with Title::newFromText() and obtain the namespace with ->getNamespace() . You're already constructing Title objects in getContent() anyway. Maybe my third comment makes more sense if you reread it with keeping this in mind?

#Comment by Tim Starling (talk | contribs)   06:35, 8 February 2011

Done in r81692.

Status & tagging log