r68661 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r68660‎ | r68661 | r68662 >
Date:14:41, 28 June 2010
Author:daniel
Status:ok (Comments)
Tags:
Comment:
improved meta-data and error handling and persistance, as suggested by tim in a comment to bug 23258
Modified paths:
  • /trunk/extensions/PagedTiffHandler/PagedTiffHandler.image.php (modified) (history)
  • /trunk/extensions/PagedTiffHandler/PagedTiffHandler_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/PagedTiffHandler/PagedTiffHandler_body.php
——@@ -97,14 +97,16 @@
9898
9999 static function extCheck( $meta, &$error, $saveName = '' ) {
100100 global $wgTiffMaxEmbedFiles, $wgTiffMaxMetaSize;
101 - if ( isset( $meta['errors'] ) ) {
 101+
 102+ $errors = pagedTiffHandler::getMetadataErrors( $meta );
 103+ if ( $errors ) {
102104 $error = 'tiff_bad_file';
103105
104106 // NOTE: in future, it will become possible to pass parameters
105 - // $error = array( 'tiff_bad_file' , join('<br />', $meta['errors']) );
 107+ // $error = array( 'tiff_bad_file' , pagedTiffHandler::joinMessages( $errors ) );
106108 // does that work now? ^DK
107109
108 - wfDebug( __METHOD__ . ": $error ($saveName) " . join( '; ', $meta['errors'] ) . "\n" );
 110+ wfDebug( __METHOD__ . ": $error ($saveName) " . pagedTiffHandler::joinMessages( $errors, false ) . "\n" );
109111 return false;
110112 }
111113 if ( ( strlen( serialize( $meta ) ) + 1 ) > $wgTiffMaxMetaSize ) {
——@@ -228,6 +230,41 @@
229231 return true;
230232 }
231233
 234+ static function getMetadataErrors( $metadata ) {
 235+ if ( is_string( $metadata ) ) {
 236+ $metadata = unserialize( $metadata );
 237+ }
 238+
 239+ if ( !$metadata ) {
 240+ return true;
 241+ } else if ( isset( $metadata[ 'errors' ] ) ) {
 242+ return $metadata[ 'errors' ];
 243+ } else {
 244+ return false;
 245+ }
 246+ }
 247+
 248+ static function joinMessages( $errors_raw, $to_html = true ) {
 249+ if ( is_array( $errors_raw ) ) {
 250+ if ( !$errors_raw ) {
 251+ return false;
 252+ }
 253+
 254+ if ( $to_html ) {
 255+ $errors = array();
 256+ foreach ( $errors_raw as $error ) {
 257+ $errors[] = htmlspecialchars( $error );
 258+ }
 259+
 260+ return join( '<br />', $errors );
 261+ } else {
 262+ return join( ";\n", $errors_raw );
 263+ }
 264+ }
 265+
 266+ return $errors_raw;
 267+ }
 268+
232269 /**
233270 * Checks whether a thumbnail with the requested file type and resolution exists,
234271 * creates it if necessary, unless self::TRANSFORM_LATER is set in $flags.
——@@ -235,15 +272,19 @@
236273 */
237274 function doTransform( $image, $dstPath, $dstUrl, $params, $flags = 0 ) {
238275 global $wgImageMagickConvertCommand, $wgTiffMaxEmbedFileResolution, $wgTiffUseVips, $wgTiffVipsCommand;
239 - $metadata = $image->getMetadata();
240276
241 - if ( !$metadata ) {
242 - if ( $metadata == - 1 ) {
243 - return $this->doThumbError( $params['width'], $params['height'], 'tiff_error_cached' ); # #test this ^DK
244 - // $wgCacheType = CACHE_DB
 277+ $meta = $this->getMetaArray( $image );
 278+ $errors = PagedTiffHandler::getMetadataErrors( $meta );
 279+
 280+ if ( $errors ) {
 281+ $errors = PagedTiffHandler::joinMessages( $errors );
 282+ if ( is_string( $errors ) ) {
 283+ return $this->doThumbError( $params, 'tiff_bad_file' ); #TODO: original error as param # #test this ^DK
 284+ } else {
 285+ return $this->doThumbError( $params, 'tiff_no_metadata' );
245286 }
246 - return $this->doThumbError( $params['width'], $params['height'], 'tiff_no_metadata' );
247287 }
 288+
248289 if ( !$this->normaliseParams( $image, $params ) )
249290 return new TransformParameterError( $params );
250291
——@@ -257,24 +298,23 @@
258299 return new ThumbnailImage( $image, $dstUrl, $width, $height, $dstPath, $page );
259300 }
260301
261 - $meta = unserialize( $metadata );
262 -
263302 if ( !$this->extCheck( $meta, $error, $dstPath ) ) {
264 - return $this->doThumbError( $params['width'], $params['height'], $error );
 303+ return $this->doThumbError( $params, $error );
265304 }
266305
267 - if ( is_file( $dstPath ) )
 306+ if ( is_file( $dstPath ) ) {
268307 return new ThumbnailImage( $image, $dstUrl, $width,
269308 $height, $dstPath, $page );
 309+ }
270310
271311 if ( isset( $meta['page_data'][$page]['pixels'] ) && $meta['page_data'][$page]['pixels'] > $wgTiffMaxEmbedFileResolution )
272 - return $this->doThumbError( $width, $height, 'tiff_sourcefile_too_large' );
 312+ return $this->doThumbError( $params, 'tiff_sourcefile_too_large' );
273313
274314 if ( ( $width * $height ) > $wgTiffMaxEmbedFileResolution )
275 - return $this->doThumbError( $width, $height, 'tiff_targetfile_too_large' );
 315+ return $this->doThumbError( $params, 'tiff_targetfile_too_large' );
276316
277317 if ( !wfMkdirParents( dirname( $dstPath ) ) )
278 - return $this->doThumbError( $width, $height, 'thumbnail_dest_directory' );
 318+ return $this->doThumbError( $params, 'thumbnail_dest_directory' );
279319
280320 if ( $wgTiffUseVips ) {
281321 // tested in Linux
——@@ -333,7 +373,28 @@
334374 /**
335375 * Returns a new error message.
336376 */
337 - protected function doThumbError( $width, $height, $msg ) {
 377+ protected function doThumbError( $params, $msg ) {
 378+ global $wgUser, $wgThumbLimits;
 379+
 380+ if ( empty( $params['width'] ) ) {
 381+ // no width/height in the parameter array
 382+ // only happens if we don't have image meta-data, and no
 383+ // size was specified by the user.
 384+ // we need to pick *some* size, and the preferred
 385+ // thumbnail size seems sane.
 386+ $sz = $wgUser->getOption( 'thumbsize' );
 387+ $width = $wgThumbLimits[ $sz ];
 388+ $height = $width; // we don't have a hight, and no aspect ratio. make it square.
 389+ } else {
 390+ $width = intval( $params['width'] );
 391+
 392+ if ( !empty( $params['height'] ) ) {
 393+ $height = intval( $params['height'] );
 394+ } else {
 395+ $height = $width; // we don't have a hight, and no aspect ratio. make it square.
 396+ }
 397+ }
 398+
338399 wfLoadExtensionMessages( 'PagedTiffHandler' );
339400 return new MediaTransformError( 'thumbnail_error',
340401 $width, $height, wfMsg( $msg ) );
——@@ -383,8 +444,14 @@
384445 * If it returns false, Image will reload the metadata from the file and update the database
385446 */
386447 function isMetadataValid( $image, $metadata ) {
 448+
387449 if ( !empty( $metadata ) && $metadata != serialize( array() ) ) {
388450 $meta = unserialize( $metadata );
 451+ if ( isset( $meta['errors'] ) ) {
 452+ //metadata contains an error message, but it's valid.
 453+ //don't try to re-render until the error is resolved!
 454+ return true;
 455+ }
389456 if ( isset( $meta['page_amount'] ) && isset( $meta['page_data'] ) ) {
390457 return true;
391458 }
——@@ -461,28 +528,23 @@
462529 );
463530 }
464531 $meta = unserialize( $metadata );
465 - if ( isset( $meta['errors'] ) ) {
466 - $errors = array();
467 - foreach ( $meta['errors'] as $error ) {
468 - $errors[] = htmlspecialchars( $error );
469 - }
 532+ $errors_raw = PagedTiffHandler::getMetadataErrors( $meta );
 533+ if ( $errors_raw ) {
 534+ $errors = PagedTiffHandler::joinMessages( $errors_raw );
470535 self::addMeta( $result,
471536 'collapsed',
472537 'identify',
473538 'error',
474 - join( '<br />', $errors )
 539+ $errors
475540 );
476541 }
477542 if ( isset( $meta['warnings'] ) ) {
478 - $warnings = array();
479 - foreach ( $meta['warnings'] as $warning ) {
480 - $warnings[] = htmlspecialchars( $warning );
481 - }
 543+ $warnings = PagedTiffHandler::joinMessages( $meta['warnings'] );
482544 self::addMeta( $result,
483545 'collapsed',
484546 'identify',
485547 'warning',
486 - join( '<br />', $warnings )
 548+ $warnings
487549 );
488550 }
489551 return $result;
Index: trunk/extensions/PagedTiffHandler/PagedTiffHandler.image.php
——@@ -80,15 +80,8 @@
8181 * meta['warnings'] = identify-warnings
8282 */
8383 public function retrieveMetaData() {
84 - global $wgImageMagickIdentifyCommand, $wgTiffExivCommand, $wgTiffUseExiv, $wgMemc, $wgTiffErrorCacheTTL;
 84+ global $wgImageMagickIdentifyCommand, $wgTiffExivCommand, $wgTiffUseExiv;
8585
86 - $imgKey = wfMemcKey( 'PagedTiffHandler-ThumbnailGeneration', $this->mFilename );
87 - $isCached = $wgMemc->get( $imgKey );
88 - if ( $isCached ) {
89 - return - 1;
90 - }
91 - $wgMemc->add( $imgKey, 1, $wgTiffErrorCacheTTL );
92 -
9386 if ( $this->_meta === null ) {
9487 if ( $wgImageMagickIdentifyCommand ) {
9588
——@@ -106,7 +99,9 @@
107100 $dump = wfShellExec( $cmd, $retval );
108101 wfProfileOut( 'identify' );
109102 if ( $retval ) {
110 - return false;
 103+ $data['errors'][] = "identify command failed: $cmd";
 104+ wfDebug( __METHOD__ . ": identify command failed: $cmd\n" );
 105+ return $data; //fail. we *need* that info
111106 }
112107 $this->_meta = $this->convertDumpToArray( $dump );
113108 $this->_meta['exif'] = array();
——@@ -124,6 +119,13 @@
125120 wfDebug( __METHOD__ . ": $cmd\n" );
126121 $dump = wfShellExec( $cmd, $retval );
127122 wfProfileOut( 'exiv2' );
 123+
 124+ if ( $retval ) {
 125+ $data['errors'][] = "exiv command failed: $cmd";
 126+ wfDebug( __METHOD__ . ": exiv command failed: $cmd\n" );
 127+ //don't fail - we are missing info, but that's no reason to abort yet.
 128+ }
 129+
128130 $result = array();
129131 preg_match_all( '/(\w+)\s+(.+)/', $dump, $result, PREG_SET_ORDER );
130132
——@@ -139,6 +141,13 @@
140142 wfDebug( __METHOD__ . ": $cmd\n" );
141143 $dump = wfShellExec( $cmd, $retval );
142144 wfProfileOut( 'identify -verbose' );
 145+
 146+ if ( $retval ) {
 147+ $data['errors'][] = "identify command failed: $cmd";
 148+ wfDebug( __METHOD__ . ": identify command failed: $cmd\n" );
 149+ //don't fail - we are missing info, but that's no reason to abort yet.
 150+ }
 151+
143152 $this->_meta['exif'] = $this->parseVerbose( $dump );
144153 }
145154 wfProfileOut( 'PagedTiffImage::retrieveMetaData' );
——@@ -156,12 +165,15 @@
157166 */
158167 protected function convertDumpToArray( $dump ) {
159168 global $wgTiffIdentifyRejectMessages, $wgTiffIdentifyBypassMessages;
 169+
 170+ $data = array();
160171 if ( strval( $dump ) == '' ) {
161 - return false;
 172+ $data['errors'][] = "no metadata";
 173+ return $data;
162174 }
 175+
163176 $infos = null;
164177 preg_match_all( '/\[BEGIN\](.+?)\[END\]/si', $dump, $infos, PREG_SET_ORDER );
165 - $data = array();
166178 $data['page_amount'] = count( $infos );
167179 $data['page_data'] = array();
168180 foreach ( $infos as $info ) {
——@@ -216,7 +228,7 @@
217229 }
218230 }
219231 if ( !$knownError ) {
220 - $data['warning'][] = $error;
 232+ $data['warnings'][] = $error;
221233 }
222234 }
223235 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r68664use internal Exif per default, as suggested by tim in a comment to bug 23258;...daniel16:15, 28 June 2010
r68711fix caps: pagedTiffHandler -> PagedTiffHandler; follow-up to r68661daniel07:46, 29 June 2010
r68712style and comments; follow-up to r68661daniel08:04, 29 June 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r68409passing thumbnail params to getThumbType, as suggested by TimStarling in a co...daniel16:01, 22 June 2010
r68418fixed thumbnail parameter handling, as suggested by TimStarling in a comment ...daniel17:40, 22 June 2010

Comments

#Comment by Tim Starling (talk | contribs)   04:18, 29 June 2010

Class names start with a capital letter. Two instances. Marking fixme for this. 

+		if ( empty( $params['width'] ) ) {
+			// no width/height in the parameter array

Questionable use of empty(), function does not match comment. Consider isset().

Consider fixing the comments to conform with Manual:Coding conventions#Spaces, i.e. with a space between the // and the text. Also, move or reformat the end-of-line comments to avoid overrunning the recommended line length limit of 100, e.g. 

+				return $this->doThumbError( $params, 'tiff_bad_file' ); #TODO: original error as param # #test this ^DK

Goes to column 120 with 4 spaces per tab.

Otherwise good.

#Comment by Duesentrieb (talk | contribs)   08:08, 29 June 2010

fixed class names in r68711.

kept empty() in favour of isset(), since we do indeed a non-null, non-zero value here. adjusted the comment.

fixed comment style and some line wrapping in r68712.

Setting status to "new".

Status & tagging log