r78924 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r78923‎ | r78924 | r78925 >
Date:20:14, 23 December 2010
Author:catrope
Status:resolved
Tags:
Comment:
Proper exception handling in ResourceLoader. Catch exceptions and output them in comments. Instead of just surrounding everything in a big try { } block, catch module-level exceptions where possible to allow error-free modules loaded in the same request to still be output normally. Modules that don't get output because of an exception are marked as missing in the client-side loader so other modules depending on them won't get loaded either.
Modified paths:
  • /trunk/phase3/includes/resourceloader/ResourceLoader.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/resourceloader/ResourceLoader.php
@@ -66,7 +66,7 @@
6767 ), __METHOD__
6868 );
6969
70 - // Set modules' dependecies
 70+ // Set modules' dependencies
7171 $modulesWithDeps = array();
7272 foreach ( $res as $row ) {
7373 $this->getModule( $row->md_module )->setFileDependencies( $skin,
@@ -117,7 +117,7 @@
118118 *
119119 * @param $filter String: Name of filter to run
120120 * @param $data String: Text to filter, such as JavaScript or CSS text
121 - * @return String: Filtered data
 121+ * @return String: Filtered data, or a comment containing an error message
122122 */
123123 protected function filter( $filter, $data ) {
124124 wfProfileIn( __METHOD__ );
@@ -151,14 +151,14 @@
152152 $result = CSSMin::minify( $data );
153153 break;
154154 }
 155+
 156+ // Save filtered text to Memcached
 157+ $cache->set( $key, $result );
155158 } catch ( Exception $exception ) {
156 - throw new MWException( 'ResourceLoader filter error. ' .
157 - 'Exception was thrown: ' . $exception->getMessage() );
 159+ // Return exception as a comment
 160+ $result = "/*\n{$exception->__toString()}\n*/";
158161 }
159162
160 - // Save filtered text to Memcached
161 - $cache->set( $key, $result );
162 -
163163 wfProfileOut( __METHOD__ );
164164
165165 return $result;
@@ -294,6 +294,7 @@
295295 ob_start();
296296
297297 wfProfileIn( __METHOD__ );
 298+ $response = '';
298299
299300 // Split requested modules into two groups, modules and missing
300301 $modules = array();
@@ -320,7 +321,12 @@
321322 }
322323
323324 // Preload information needed to the mtime calculation below
324 - $this->preloadModuleInfo( array_keys( $modules ), $context );
 325+ try {
 326+ $this->preloadModuleInfo( array_keys( $modules ), $context );
 327+ } catch( Exception $e ) {
 328+ // Add exception to the output as a comment
 329+ $response .= "/*\n{$e->__toString()}\n*/";
 330+ }
325331
326332 wfProfileIn( __METHOD__.'-getModifiedTime' );
327333
@@ -328,12 +334,17 @@
329335 // the last modified time
330336 $mtime = wfTimestamp( TS_UNIX, $wgCacheEpoch );
331337 foreach ( $modules as $module ) {
332 - // Bypass squid cache if the request includes any private modules
333 - if ( $module->getGroup() === 'private' ) {
334 - $smaxage = 0;
 338+ try {
 339+ // Bypass squid cache if the request includes any private modules
 340+ if ( $module->getGroup() === 'private' ) {
 341+ $smaxage = 0;
 342+ }
 343+ // Calculate maximum modified time
 344+ $mtime = max( $mtime, $module->getModifiedTime( $context ) );
 345+ } catch ( Exception $e ) {
 346+ // Add exception to the output as a comment
 347+ $response .= "/*\n{$e->__toString()}\n*/";
335348 }
336 - // Calculate maximum modified time
337 - $mtime = max( $mtime, $module->getModifiedTime( $context ) );
338349 }
339350
340351 wfProfileOut( __METHOD__.'-getModifiedTime' );
@@ -379,7 +390,7 @@
380391 }
381392
382393 // Generate a response
383 - $response = $this->makeModuleResponse( $context, $modules, $missing );
 394+ $response .= $this->makeModuleResponse( $context, $modules, $missing );
384395
385396 // Capture any PHP warnings from the output buffer and append them to the
386397 // response in a comment if we're in debug mode.
@@ -405,62 +416,73 @@
406417 public function makeModuleResponse( ResourceLoaderContext $context,
407418 array $modules, $missing = array() )
408419 {
 420+ $out = '';
409421 if ( $modules === array() && $missing === array() ) {
410422 return '/* No modules requested. Max made me put this here */';
411423 }
412424
413425 // Pre-fetch blobs
414426 if ( $context->shouldIncludeMessages() ) {
415 - $blobs = MessageBlobStore::get( $this, $modules, $context->getLanguage() );
 427+ //try {
 428+ $blobs = MessageBlobStore::get( $this, $modules, $context->getLanguage() );
 429+ //} catch ( Exception $e ) {
 430+ // Add exception to the output as a comment
 431+ // $out .= "/*\n{$e->__toString()}\n*/";
 432+ //}
416433 } else {
417434 $blobs = array();
418435 }
419436
420437 // Generate output
421 - $out = '';
422438 foreach ( $modules as $name => $module ) {
423 -
424439 wfProfileIn( __METHOD__ . '-' . $name );
 440+ try {
 441+ // Scripts
 442+ $scripts = '';
 443+ if ( $context->shouldIncludeScripts() ) {
 444+ $scripts .= $module->getScript( $context ) . "\n";
 445+ }
425446
426 - // Scripts
427 - $scripts = '';
428 - if ( $context->shouldIncludeScripts() ) {
429 - $scripts .= $module->getScript( $context ) . "\n";
430 - }
 447+ // Styles
 448+ $styles = array();
 449+ if ( $context->shouldIncludeStyles() ) {
 450+ $styles = $module->getStyles( $context );
 451+ }
431452
432 - // Styles
433 - $styles = array();
434 - if ( $context->shouldIncludeStyles() ) {
435 - $styles = $module->getStyles( $context );
436 - }
 453+ // Messages
 454+ $messagesBlob = isset( $blobs[$name] ) ? $blobs[$name] : '{}';
437455
438 - // Messages
439 - $messagesBlob = isset( $blobs[$name] ) ? $blobs[$name] : '{}';
 456+ // Append output
 457+ switch ( $context->getOnly() ) {
 458+ case 'scripts':
 459+ $out .= $scripts;
 460+ break;
 461+ case 'styles':
 462+ $out .= self::makeCombinedStyles( $styles );
 463+ break;
 464+ case 'messages':
 465+ $out .= self::makeMessageSetScript( new XmlJsCode( $messagesBlob ) );
 466+ break;
 467+ default:
 468+ // Minify CSS before embedding in mediaWiki.loader.implement call
 469+ // (unless in debug mode)
 470+ if ( !$context->getDebug() ) {
 471+ foreach ( $styles as $media => $style ) {
 472+ $styles[$media] = $this->filter( 'minify-css', $style );
 473+ }
 474+ }
 475+ $out .= self::makeLoaderImplementScript( $name, $scripts, $styles,
 476+ new XmlJsCode( $messagesBlob ) );
 477+ break;
 478+ }
 479+ } catch ( Exception $e ) {
 480+ // Add exception to the output as a comment
 481+ $out .= "/*\n{$e->__toString()}\n*/";
440482
441 - // Append output
442 - switch ( $context->getOnly() ) {
443 - case 'scripts':
444 - $out .= $scripts;
445 - break;
446 - case 'styles':
447 - $out .= self::makeCombinedStyles( $styles );
448 - break;
449 - case 'messages':
450 - $out .= self::makeMessageSetScript( new XmlJsCode( $messagesBlob ) );
451 - break;
452 - default:
453 - // Minify CSS before embedding in mediaWiki.loader.implement call
454 - // (unless in debug mode)
455 - if ( !$context->getDebug() ) {
456 - foreach ( $styles as $media => $style ) {
457 - $styles[$media] = $this->filter( 'minify-css', $style );
458 - }
459 - }
460 - $out .= self::makeLoaderImplementScript( $name, $scripts, $styles,
461 - new XmlJsCode( $messagesBlob ) );
462 - break;
 483+ // Register module as missing
 484+ $missing[] = $name;
 485+ unset( $modules[$name] );
463486 }
464 -
465487 wfProfileOut( __METHOD__ . '-' . $name );
466488 }
467489

Follow-up revisions

RevisionCommit summaryAuthorDate
r79139MFT r78924platonides22:28, 28 December 2010
r79891Followup r78924: keep track of exception/warning comments separately, to prev...catrope12:29, 9 January 2011

Status & tagging log