r69432 MediaWiki - Code Review archive

+ * @author Roan Kattouw, Trevor Parscal

Doxygen handles multiple authorship simply with multiple @author statements.

+				$cssJanus = new CSSJanus();
+				$result = $cssJanus::transform( $data, true, false );

WTH? Just use CSSJanus::transform().

+			case 'strip-debug':
+				$result = preg_replace( '/\n\s*mw\.log\(([^\)]*\))*\s*[\;\n]/U', "\n", $data );
+				break;

I've expressed my reservations with this in an earlier CR.

+				foreach ( ResourceLoader::$modules[$module]['messages'] as $key ) {

Now that this has been moved to the ResourceLoader class, it should use self::.

+				if ( !static::register( $name, $options ) ) {

This breaks in PHP < 5.3 , use self::. More such usages elsewhere in this file.

+	 * @param array $modules module names to include in the request
+	 * @param array $options options which affect the content of the response (optional)
[...]
+	public static function respond( WebRequest $request ) {

Documentation doesn't match module signature+implementation. I really, really, prefer the documented form over the implemented form because I know what a pain WebRequest-reliant functions are from first-hand experience.

+		$parameters = array_merge( array(
+			'user' => $request->getBool( 'user', $wgUser->isLoggedIn() ),
+			'lang' => $request->getVal( 'lang', $wgLang->getCode() ),
+			'skin' => $request->getVal( 'skin', $wgDefaultSkin ),
+			'base' => $request->getBool( 'base' ),
+			'debug' => $request->getBool( 'debug' ),
+		) );

Calling array_merge() with one argument isn't very useful.

				if ( $options['base'] ) {
+					// Only include debug scripts in debug mode
+					if ( $options['debug'] ) {
+						if ( $parameters['debug'] ) {
+							$base[] = $name;
+						}
+					} else {
+						$base[] = $name;
+					}
+				}

Can be shortened to if ( $options['base'] && ( !$options['debug'] || $parameters['debug'] ) ) { $base[] = $name; }.

+		// Include requested modules which have been registered - ignoring any which have not been
+		$other = array();
+		foreach ( static::$modules as $name => $options ) {
+			if ( in_array( $name, $modules ) && !in_array( $name, $base )) {
+				$other[] = $name;
+			}
+		}

Misses check for non-base debug modules, which are not ruled out by the documentation.

+		if ( $parameters['base'] ) {
[...]
+			// All module loaders - keep track of which loaders have been included to prevent multiple modules with a
+			// single loader causing the loader to be included more than once
+			$loaders = array();
+			foreach ( self::$modules as $name => $options ) {
[include loaders]

So the base parameter triggers the inclusion of all loaders. This makes sense to me personally, but is not documented.

+				$style = Xml::escapeJsString(
+					static::filter( 'minify-css', $style, static::$modules[$module]['style'] )
+				);

You're minifying CSS unconditionally, even in debug mode, whereas you are taking care not to minify JS in debug mode.

+			$messages = isset( $blobs[$module] ) ? $blobs[$module] : '{}';
+			// Output
+			echo "mw.loader.implement( '{$module}', function() { {$script} }, '{$style}', {$messages} );\n";

This produces {{}} if $blobs[$module] is not set. : '{}' should be : ''.

r69563 fixes most of these. The ones left over are:

• Weird static call on an object to CSSJanus::transform()
• {{}} output if blob not set

Why did you use an output buffer to accumulate the output string instead of a variable? The code would have been almost identical, just $s.=... instead of echo ..., but variables don't screw up error reporting like output buffers do.

This was originally done so we could use some function (forgot the name) that reads a file and writes it straight to the output, but since we process all file contents now we no longer do that. Accumulating output in a variable would also allow us to clear any error reporting from the output before writing it out: we've had quite a few bugs where the layout was messed up because a PHP warning appeared at the top of a CSS file and made the entire thing invalid CSS. Filed bug 25200 for myself.

I agree that this happened because we were using readfile() a few places, but now everything is abstracted a few layers away, it makes much less sense.

Output buffering is not used anymore.