r75036 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r75035‎ | r75036 | r75037 >
Date:20:59, 19 October 2010
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
Fixed ResourceLoaderFileModule constructor - collated lists were not being prefixed properly. Also did some renaming to improve consistency.
Modified paths:
  • /trunk/phase3/includes/resourceloader/ResourceLoaderFileModule.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/resourceloader/ResourceLoaderFileModule.php
@@ -26,6 +26,7 @@
2727 * Module based on local JS/CSS files. This is the most common type of module.
2828 */
2929 class ResourceLoaderFileModule extends ResourceLoaderModule {
 30+
3031 /* Protected Members */
3132
3233 protected $scripts = array();
@@ -38,7 +39,6 @@
3940 protected $skinScripts = array();
4041 protected $skinStyles = array();
4142 protected $loaders = array();
42 - protected $parameters = array();
4343
4444 // In-object cache for file dependencies
4545 protected $fileDeps = array();
@@ -49,75 +49,65 @@
5050
5151 /**
5252 * Construct a new module from an options array.
53 - *
54 - * @param $options array Options array. If empty, an empty module will be constructed
55 - *
56 - * $options format:
 53+ *
 54+ * @param {array} $options Options array. If not given or empty, an empty module will be constructed
 55+ * @param {string} $basePath base path to prepend to all paths in $options
 56+ *
 57+ * @format $options
5758 * array(
58 - * // Required module options (mutually exclusive)
59 - * 'scripts' => 'dir/script.js' | array( 'dir/script1.js', 'dir/script2.js' ... ),
60 - *
61 - * // Optional module options
 59+ * // Scripts to always include
 60+ * 'scripts' => [file path string or array of file path strings],
 61+ * // Scripts to include in specific language contexts
6262 * 'languageScripts' => array(
63 - * '[lang name]' => 'dir/lang.js' | '[lang name]' => array( 'dir/lang1.js', 'dir/lang2.js' ... )
64 - * ...
 63+ * [language code] => [file path string or array of file path strings],
6564 * ),
66 - * 'skinScripts' => 'dir/skin.js' | array( 'dir/skin1.js', 'dir/skin2.js' ... ),
67 - * 'debugScripts' => 'dir/debug.js' | array( 'dir/debug1.js', 'dir/debug2.js' ... ),
68 - *
69 - * // Non-raw module options
70 - * 'dependencies' => 'module' | array( 'module1', 'module2' ... )
71 - * 'loaderScripts' => 'dir/loader.js' | array( 'dir/loader1.js', 'dir/loader2.js' ... ),
72 - * 'styles' => 'dir/file.css' | array( 'dir/file1.css', 'dir/file2.css' ... ), |
73 - * array( 'dir/file1.css' => array( 'media' => 'print' ) ),
 65+ * // Scripts to include in specific skin contexts
 66+ * 'skinScripts' => array(
 67+ * [skin name] => [file path string or array of file path strings],
 68+ * ),
 69+ * // Scripts to include in debug contexts
 70+ * 'debugScripts' => [file path string or array of file path strings],
 71+ * // Scripts to include in the startup module
 72+ * 'loaders' => [file path string or array of file path strings],
 73+ * // Modules which must be loaded before this module
 74+ * 'dependencies' => [modile name string or array of module name strings],
 75+ * // Styles to always load
 76+ * 'styles' => [file path string or array of file path strings],
 77+ * // Styles to include in specific skin contexts
7478 * 'skinStyles' => array(
75 - * '[skin name]' => 'dir/skin.css' | array( 'dir/skin1.css', 'dir/skin2.css' ... ) |
76 - * array( 'dir/file1.css' => array( 'media' => 'print' )
77 - * ...
 79+ * [skin name] => [file path string or array of file path strings],
7880 * ),
79 - * 'messages' => array( 'message1', 'message2' ... ),
80 - * 'group' => 'stuff',
 81+ * // Messages to always load
 82+ * 'messages' => [array of message key strings],
 83+ * // Group which this module should be loaded together with
 84+ * 'group' => [group name string],
8185 * )
82 - *
83 - * @param $basePath String: base path to prepend to all paths in $options
8486 */
8587 public function __construct( $options = array(), $basePath = null ) {
86 - foreach ( $options as $option => $value ) {
87 - switch ( $option ) {
 88+ foreach ( $options as $member => $option ) {
 89+ switch ( $member ) {
 90+ // Lists of file paths
8891 case 'scripts':
8992 case 'debugScripts':
 93+ case 'loaders':
 94+ case 'styles':
 95+ $this->{$member} = $this->prefixFilePathList( (array) $option, $basePath );
 96+ break;
 97+ // Collated lists of file paths
9098 case 'languageScripts':
9199 case 'skinScripts':
92 - case 'loaders':
93 - $this->{$option} = (array)$value;
94 - // Automatically prefix script paths
95 - if ( is_string( $basePath ) ) {
96 - foreach ( $this->{$option} as $key => $value ) {
97 - $this->{$option}[$key] = $basePath . $value;
98 - }
99 - }
100 - break;
101 - case 'styles':
102100 case 'skinStyles':
103 - $this->{$option} = (array)$value;
104 - // Automatically prefix style paths
105 - if ( is_string( $basePath ) ) {
106 - foreach ( $this->{$option} as $key => $value ) {
107 - if ( is_array( $value ) ) {
108 - $this->{$option}[$basePath . $key] = $value;
109 - unset( $this->{$option}[$key] );
110 - } else {
111 - $this->{$option}[$key] = $basePath . $value;
112 - }
113 - }
 101+ foreach ( (array) $option as $key => $value ) {
 102+ $this->{$member}[$key] = $this->prefixFilePathList( (array) $value, $basePath );
114103 }
115 - break;
 104+ // Lists of strings
116105 case 'dependencies':
117106 case 'messages':
118 - $this->{$option} = (array)$value;
 107+ $this->{$member} = (array) $option;
119108 break;
 109+ // Single strings
120110 case 'group':
121 - $this->group = (string)$value;
 111+ $this->group = (string) $option;
122112 break;
123113 }
124114 }
@@ -338,11 +328,11 @@
339329
340330 // Sort of nasty way we can get a flat list of files depended on by all styles
341331 $styles = array();
342 - foreach ( self::organizeFilesByOption( $this->styles, 'media', 'all' ) as $styleFiles ) {
 332+ foreach ( self::collateFilePathListByOption( $this->styles, 'media', 'all' ) as $styleFiles ) {
343333 $styles = array_merge( $styles, $styleFiles );
344334 }
345335 $skinFiles = (array) self::getSkinFiles(
346 - $context->getSkin(), self::organizeFilesByOption( $this->skinStyles, 'media', 'all' )
 336+ $context->getSkin(), self::collateFilePathListByOption( $this->skinStyles, 'media', 'all' )
347337 );
348338 foreach ( $skinFiles as $styleFiles ) {
349339 $styles = array_merge( $styles, $styleFiles );
@@ -371,6 +361,53 @@
372362 /* Protected Members */
373363
374364 /**
 365+ * Prefixes each file path in a list
 366+ *
 367+ * @param {array} $list List of file paths in any combination of index/path or path/options pairs
 368+ * @param {string} $prefix String to prepend to each file path in $list
 369+ * @return {array} List of prefixed file paths
 370+ */
 371+ protected function prefixFilePathList( array $list, $prefix ) {
 372+ $prefixed = array();
 373+ foreach ( $list as $key => $value ) {
 374+ if ( is_array( $value ) ) {
 375+ // array( [path] => array( [options] ) )
 376+ $prefixed[$prefix . $key] = $value;
 377+ } else {
 378+ // array( [path] )
 379+ $prefixed[$key] = $prefix . $value;
 380+ }
 381+ }
 382+ return $prefixed;
 383+ }
 384+
 385+ /**
 386+ * Collates file paths by option (where provided)
 387+ *
 388+ * @param {array} $list List of file paths in any combination of index/path or path/options pairs
 389+ */
 390+ protected static function collateFilePathListByOption( array $list, $option, $default ) {
 391+ $collatedFiles = array();
 392+ foreach ( (array) $list as $key => $value ) {
 393+ if ( is_int( $key ) ) {
 394+ // File name as the value
 395+ if ( !isset( $collatedFiles[$default] ) ) {
 396+ $collatedFiles[$default] = array();
 397+ }
 398+ $collatedFiles[$default][] = $value;
 399+ } else if ( is_array( $value ) ) {
 400+ // File name as the key, options array as the value
 401+ $media = isset( $value[$option] ) ? $value[$option] : $default;
 402+ if ( !isset( $collatedFiles[$media] ) ) {
 403+ $collatedFiles[$media] = array();
 404+ }
 405+ $collatedFiles[$media][] = $key;
 406+ }
 407+ }
 408+ return $collatedFiles;
 409+ }
 410+
 411+ /**
375412 * Get the primary JS for this module. This is pulled from the
376413 * script files added through addScripts()
377414 *
@@ -468,27 +505,6 @@
469506 array_unique( (array) $files ) ) ) );
470507 }
471508
472 - protected static function organizeFilesByOption( $files, $option, $default ) {
473 - $organizedFiles = array();
474 - foreach ( (array) $files as $key => $value ) {
475 - if ( is_int( $key ) ) {
476 - // File name as the value
477 - if ( !isset( $organizedFiles[$default] ) ) {
478 - $organizedFiles[$default] = array();
479 - }
480 - $organizedFiles[$default][] = $value;
481 - } else if ( is_array( $value ) ) {
482 - // File name as the key, options array as the value
483 - $media = isset( $value[$option] ) ? $value[$option] : $default;
484 - if ( !isset( $organizedFiles[$media] ) ) {
485 - $organizedFiles[$media] = array();
486 - }
487 - $organizedFiles[$media][] = $key;
488 - }
489 - }
490 - return $organizedFiles;
491 - }
492 -
493509 /**
494510 * Get the contents of a set of CSS files, remap then and concatenate
495511 * them, with newlines in between. Each file is used only once.
@@ -497,7 +513,7 @@
498514 * @return Array: list of concatenated and remapped contents of $files keyed by media type
499515 */
500516 protected static function concatStyles( $styles ) {
501 - $styles = self::organizeFilesByOption( $styles, 'media', 'all' );
 517+ $styles = self::collateFilePathListByOption( $styles, 'media', 'all' );
502518 foreach ( $styles as $media => $files ) {
503519 $styles[$media] =
504520 implode( "\n",

Follow-up revisions

RevisionCommit summaryAuthorDate
r75107* Improves on r75054 and r75036 by adding comments and renaming variables...tparscal20:43, 20 October 2010
r76119Fixed Doxygen incompatible JSDoc style comments (bad Trevor!) as per some com...tparscal18:33, 5 November 2010

Comments

#Comment by Catrope (talk | contribs)   15:05, 20 October 2010
+	/**
+	 * Collates file paths by option (where provided)
+	 * 
+	 * @param {array} $list List of file paths in any combination of index/path or path/options pairs
+	 */
+	protected static function collateFilePathListByOption( array $list, $option, $default ) {

How about documenting the other params and the return value too?

+				$media = isset( $value[$option] ) ? $value[$option] : $default;
+				if ( !isset( $collatedFiles[$media] ) ) {
+					$collatedFiles[$media] = array();
+				}
+				$collatedFiles[$media][] = $key;

The naming of $media only makes sense in the context of what currently happens to be the only use case for these mappings. Viewing the function independently it's an absurd and kinda confusing name.

#Comment by Tim Starling (talk | contribs)   05:27, 5 November 2010
+	 * @param {array} $options Options array. If not given or empty, an empty module will be constructed

Is there some documentation system where specifying the type in braces is possible? It doesn't work in Doxygen, which is what I thought we were targeting with our doc comments. It treats "{array}" as the parameter name and the rest of the line including "$options" as an unstructured description.

While I'm on the subject and talking on a CR comment that you're both CC'ed on: the @var commands appear to be worse than useless, they suppress display of the description entirely. Removing the "@var" makes it work again. And @format is a non-existent command.

Doxygen is the software used by maintenance/mwdocgen.php, and it's used to automatically generate the documentation at http://svn.wikimedia.org/doc/, which is linked to from the sidebar of www.mediawiki.org. I'd assume that we had switched our documentation software and I somehow missed that discussion, except that neither of these things has changed.

http://svn.wikimedia.org/doc/classResourceLoaderFileModule.html

#Comment by Catrope (talk | contribs)   12:05, 5 November 2010

I think Trevor is targeting his own hypothetical documentation system that's half-spec'ed and unwritten?

#Comment by Trevor Parscal (WMF) (talk | contribs)   17:42, 5 November 2010

Yes, the documentation system I have written (currently scans PHP and returns JSON representation of code structure and parsed comments but still needs a nice renderer) has been written to follow more of the JSDoc syntax, but that wasn't really what I was doing this for.

#Comment by Trevor Parscal (WMF) (talk | contribs)   17:42, 5 November 2010

Ah hem... OK, so yeah, since JSDoc uses these conventions I was hoping to just use a single convention across all code - but yes, Doxygen gets a little flustered by this. I will correct this, I should have been testing, not just hoping. :(

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:34, 5 November 2010

r76119 resolves this.

Status & tagging log