Bartosz Dziewoński has uploaded a new change for review.
https://gerrit.wikimedia.org/r/172591
Change subject: ResourceLoader: Stop passing around errors as /**/-comments
......................................................................
ResourceLoader: Stop passing around errors as /**/-comments
They are prone to being stripped by CSS and JS minification and can't
be used for non-CSS non-JS responses, like images.
We've already been keeping some state related to errors in the
$hasErrors member variable, let's go all the way and replace it with
$errors array, which tracks all errors accumulated during current
respond() call. When processing completes, all errors are added to the
response, if possible.
Change-Id: I6643f010174cb1f17780622e8a63db03cffe19e1
---
M includes/resourceloader/ResourceLoader.php
1 file changed, 55 insertions(+), 46 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/91/172591/1
diff --git a/includes/resourceloader/ResourceLoader.php
b/includes/resourceloader/ResourceLoader.php
index 76acc9a..a38cd3a 100644
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -63,8 +63,11 @@
*/
protected $sources = array();
- /** @var bool */
- protected $hasErrors = false;
+ /**
+ * Errors accumulated during current respond() call.
+ * @var array
+ */
+ protected $errors = array();
/**
* Load information stored in the database about modules.
@@ -209,9 +212,7 @@
} catch ( Exception $e ) {
MWExceptionHandler::logException( $e );
wfDebugLog( 'resourceloader', __METHOD__ . ":
minification failed: $e" );
- $this->hasErrors = true;
- // Return exception as a comment
- $result = self::formatException( $e );
+ $this->errors[] = self::formatExceptionNoComment( $e );
}
wfProfileOut( __METHOD__ );
@@ -579,7 +580,6 @@
ob_start();
wfProfileIn( __METHOD__ );
- $errors = '';
// Find out which modules are missing and instantiate the others
$modules = array();
@@ -591,10 +591,7 @@
// This is a security issue, see bug 34907.
if ( $module->getGroup() === 'private' ) {
wfDebugLog( 'resourceloader',
__METHOD__ . ": request for private module '$name' denied" );
- $this->hasErrors = true;
- // Add exception to the output as a
comment
- $errors .= self::makeComment( "Cannot
show private module \"$name\"" );
-
+ $this->errors[] = "Cannot show private
module \"$name\"";
continue;
}
$modules[$name] = $module;
@@ -609,9 +606,7 @@
} catch ( Exception $e ) {
MWExceptionHandler::logException( $e );
wfDebugLog( 'resourceloader', __METHOD__ . ":
preloading module info failed: $e" );
- $this->hasErrors = true;
- // Add exception to the output as a comment
- $errors .= self::formatException( $e );
+ $this->errors[] = self::formatExceptionNoComment( $e );
}
wfProfileIn( __METHOD__ . '-getModifiedTime' );
@@ -629,9 +624,7 @@
} catch ( Exception $e ) {
MWExceptionHandler::logException( $e );
wfDebugLog( 'resourceloader', __METHOD__ . ":
calculating maximum modified time failed: $e" );
- $this->hasErrors = true;
- // Add exception to the output as a comment
- $errors .= self::formatException( $e );
+ $this->errors[] =
self::formatExceptionNoComment( $e );
}
}
@@ -649,23 +642,11 @@
// Capture any PHP warnings from the output buffer and append
them to the
// error list if we're in debug mode.
if ( $context->getDebug() && strlen( $warnings =
ob_get_contents() ) ) {
- $errors .= self::makeComment( $warnings );
- $this->hasErrors = true;
- }
-
- if ( $this->hasErrors ) {
- if ( $context->getImageObj() ) {
- // Bail, we can't show both the error messages
and the response when it's not CSS or JS.
- // sendResponseHeaders() will handle this
sensibly.
- $response = $errors;
- } else {
- // Prepend comments indicating exceptions
- $response = $errors . $response;
- }
+ $this->errors[] = $warnings;
}
// Save response to file cache unless there are errors
- if ( isset( $fileCache ) && !$this->hasErrors && !count(
$missing ) ) {
+ if ( isset( $fileCache ) && !$this->errors && !count( $missing
) ) {
// Cache single modules and images...and other requests
if there are enough hits
if ( ResourceFileCache::useFileCache( $context ) ) {
if ( $fileCache->isCacheWorthy() ) {
@@ -677,10 +658,28 @@
}
// Send content type and cache related headers
- $this->sendResponseHeaders( $context, $mtime, $this->hasErrors
);
+ $this->sendResponseHeaders( $context, $mtime,
(bool)$this->errors );
// Remove the output buffer and output the response
ob_end_clean();
+
+ if ( $context->getImageObj() && $this->errors ) {
+ // We can't show both the error messages and the
response when it's an image.
+ $errorText = '';
+ foreach ( $this->errors as $error ) {
+ $errorText .= $error . "\n";
+ }
+ $response = $errorText;
+ } elseif ( $this->errors ) {
+ // Prepend comments indicating errors
+ $errorText = '';
+ foreach ( $this->errors as $error ) {
+ $errorText .= self::makeComment( $error );
+ }
+ $response = $errorText . $response;
+ }
+
+ $this->errors = array();
echo $response;
wfProfileOut( __METHOD__ );
@@ -690,7 +689,7 @@
* Send content type and last modified headers to the client.
* @param ResourceLoaderContext $context
* @param string $mtime TS_MW timestamp to use for last-modified
- * @param bool $errors Whether there are commented-out errors in the
response
+ * @param bool $errors Whether there are errors in the response
* @return void
*/
protected function sendResponseHeaders( ResourceLoaderContext $context,
$mtime, $errors ) {
@@ -708,6 +707,7 @@
$smaxage = $rlMaxage['versioned']['server'];
}
if ( $context->getImageObj() ) {
+ // Output different headers if we're outputting textual
errors.
if ( $errors ) {
header( 'Content-Type: text/plain;
charset=utf-8' );
} else {
@@ -837,15 +837,26 @@
* Handle exception display.
*
* @param Exception $e Exception to be shown to the user
- * @return string Sanitized text that can be returned to the user
+ * @return string Sanitized text in a CSS/JS comment that can be
returned to the user
*/
public static function formatException( $e ) {
+ return self::makeComment( $this->formatExceptionNoComment( $e )
);
+ }
+
+ /**
+ * Handle exception display.
+ *
+ * @since 1.25
+ * @param Exception $e Exception to be shown to the user
+ * @return string Sanitized text that can be returned to the user
+ */
+ protected static function formatExceptionNoComment( $e ) {
global $wgShowExceptionDetails;
if ( $wgShowExceptionDetails ) {
- return self::makeComment( $e->__toString() );
+ return $e->__toString();
} else {
- return self::makeComment( wfMessage( 'internalerror'
)->text() );
+ return wfMessage( 'internalerror' )->text();
}
}
@@ -861,7 +872,6 @@
array $modules, array $missing = array()
) {
$out = '';
- $exceptions = '';
$states = array();
if ( !count( $modules ) && !count( $missing ) ) {
@@ -878,6 +888,10 @@
$context->getVariant(),
$context->getFormat()
);
+ if ( $data === false ) {
+ $data = '';
+ $this->errors[] = 'Image generation failed';
+ }
wfProfileOut( __METHOD__ );
return $data;
}
@@ -892,9 +906,7 @@
'resourceloader',
__METHOD__ . ": pre-fetching blobs from
MessageBlobStore failed: $e"
);
- $this->hasErrors = true;
- // Add exception to the output as a comment
- $exceptions .= self::formatException( $e );
+ $this->errors[] =
self::formatExceptionNoComment( $e );
}
} else {
$blobs = array();
@@ -1018,9 +1030,7 @@
} catch ( Exception $e ) {
MWExceptionHandler::logException( $e );
wfDebugLog( 'resourceloader', __METHOD__ . ":
generating module package failed: $e" );
- $this->hasErrors = true;
- // Add exception to the output as a comment
- $exceptions .= self::formatException( $e );
+ $this->errors[] =
self::formatExceptionNoComment( $e );
// Respond to client with error-state instead
of module implementation
$states[$name] = 'error';
@@ -1046,9 +1056,8 @@
}
} else {
if ( count( $states ) ) {
- $exceptions .= self::makeComment(
- 'Problematic modules: ' .
FormatJson::encode( $states, ResourceLoader::inDebugMode() )
- );
+ $this->errors[] = 'Problematic modules: ' .
+ FormatJson::encode( $states,
ResourceLoader::inDebugMode() );
}
}
@@ -1061,7 +1070,7 @@
}
wfProfileOut( __METHOD__ );
- return $exceptions . $out;
+ return $out;
}
/* Static Methods */
--
To view, visit https://gerrit.wikimedia.org/r/172591
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6643f010174cb1f17780622e8a63db03cffe19e1
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Bartosz Dziewoński <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
