From f358c115a3bb661e71f0f3e835cfd290ef1f5b00 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Gerg=C5=91=20Tisza?= <tgr.huwiki@gmail.com>
Date: Fri, 10 Aug 2018 00:11:36 +0200
Subject: [PATCH] ThumbnailRenderJob: normalize parameters before generating
 thumb URL

PagedTiffHandler in particular will fail the generate a param string
for non-normalized parameters.

Also improve logging while we are at it.

Bug: T201305
Change-Id: I40e188f6525187303b6773990b887838b80630e0
---
 includes/jobqueue/jobs/ThumbnailRenderJob.php | 66 ++++++++++++-------
 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/includes/jobqueue/jobs/ThumbnailRenderJob.php b/includes/jobqueue/jobs/ThumbnailRenderJob.php
index cf3155d7a8..49eabbba25 100644
--- a/includes/jobqueue/jobs/ThumbnailRenderJob.php
+++ b/includes/jobqueue/jobs/ThumbnailRenderJob.php
@@ -43,29 +43,18 @@ class ThumbnailRenderJob extends Job {
 			if ( $wgUploadThumbnailRenderMethod === 'jobqueue' ) {
 				$thumb = $file->transform( $transformParams, File::RENDER_NOW );
 
-				if ( $thumb && !$thumb->isError() ) {
-					return true;
-				} else {
-					$this->setLastError( __METHOD__ . ': thumbnail couln\'t be generated' );
+				if ( !$thumb || $thumb->isError() ) {
+					if ( $thumb instanceof MediaTransformError ) {
+						$this->setLastError( __METHOD__ . ': thumbnail couln\'t be generated:' .
+							$thumb->toText() );
+					} else {
+						$this->setLastError( __METHOD__ . ': thumbnail couln\'t be generated' );
+					}
 					return false;
 				}
+				return true;
 			} elseif ( $wgUploadThumbnailRenderMethod === 'http' ) {
-				$thumbUrl = '';
-				$status = $this->hitThumbUrl( $file, $transformParams, $thumbUrl );
-
-				wfDebug( __METHOD__ . ": received status {$status}\n" );
-
-				// 400 happens when requesting a size greater or equal than the original
-				if ( $status === 200 || $status === 301 || $status === 302 || $status === 400 ) {
-					return true;
-				} elseif ( $status ) {
-					$this->setLastError( __METHOD__ . ': incorrect HTTP status ' .
-						$status . ' when hitting ' . $thumbUrl );
-					return false;
-				} else {
-					$this->setLastError( __METHOD__ . ': HTTP request failure' );
-					return false;
-				}
+				return $this->hitThumbUrl( $file, $transformParams );
 			} else {
 				$this->setLastError( __METHOD__ . ': unknown thumbnail render method ' .
 					$wgUploadThumbnailRenderMethod );
@@ -77,16 +66,35 @@ class ThumbnailRenderJob extends Job {
 		}
 	}
 
-	protected function hitThumbUrl( LocalFile $file, $transformParams, &$thumbUrl ) {
+	/**
+	 * @param LocalFile $file
+	 * @param array $transformParams
+	 * @return bool Success status (error will be set via setLastError() when false)
+	 */
+	protected function hitThumbUrl( LocalFile $file, $transformParams ) {
 		global $wgUploadThumbnailRenderHttpCustomHost, $wgUploadThumbnailRenderHttpCustomDomain;
 
+		$handler = $file->getHandler();
+		if ( !$handler ) {
+			$this->setLastError( __METHOD__ . ': could not get handler' );
+			return false;
+		} elseif ( !$handler->normaliseParams( $file, $transformParams ) ) {
+			$this->setLastError( __METHOD__ . ': failed to normalize' );
+			return false;
+		}
 		$thumbName = $file->thumbName( $transformParams );
 		$thumbUrl = $file->getThumbUrl( $thumbName );
 
+		if ( $thumbUrl === null ) {
+			$this->setLastError( __METHOD__ . ': could not get thumb URL' );
+			return false;
+		}
+
 		if ( $wgUploadThumbnailRenderHttpCustomDomain ) {
 			$parsedUrl = wfParseUrl( $thumbUrl );
 
 			if ( !$parsedUrl || !isset( $parsedUrl['path'] ) || !strlen( $parsedUrl['path'] ) ) {
+				$this->setLastError( __METHOD__ . ": invalid thumb URL: $thumbUrl" );
 				return false;
 			}
 
@@ -105,7 +113,19 @@ class ThumbnailRenderJob extends Job {
 		}
 
 		$status = $request->execute();
-
-		return $request->getStatus();
+		$statusCode = $request->getStatus();
+		wfDebug( __METHOD__ . ": received status {$statusCode}\n" );
+
+		// 400 happens when requesting a size greater or equal than the original
+		// TODO use proper error signaling. 400 could mean a number of other things.
+		if ( $statusCode === 200 || $statusCode === 301 || $statusCode === 302 || $statusCode === 400 ) {
+			return true;
+		} elseif ( $statusCode ) {
+			$this->setLastError( __METHOD__ . ": incorrect HTTP status $statusCode when hitting $thumbUrl" );
+		} else {
+			$this->setLastError( __METHOD__ . ': HTTP request failure: '
+				. Status::wrap( $status )->getWikiText( null, null, 'en' ) );
+		}
+		return false;
 	}
 }
-- 
2.20.1