From: daniel <dkinzler@wikimedia.org>
Date: Fri, 30 Aug 2019 08:55:01 +0000 (+0200)
Subject: LocalFile: avoid hard failures on non-existing files.
X-Git-Tag: 1.34.0-rc.0~168
X-Git-Url: http://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=bdc6b4e378c6872a20f6fb5842f1a49961af91b4

LocalFile: avoid hard failures on non-existing files.

Some methods on LocalFile will fatal if called on a non-existing file.
ApiQueryImageInfo did not take that into account.

This patch changes LocalFile to avoid fatal errors, and ApiQueryImageInfo
to not try and report information on non-existing files.

NOTE: the modified code has NO test coverage! This should be fixed
before this patch is applied, or the patch needs to be thoroughly tested
manually.

Bug: T221812
Change-Id: I9b74545a393d1b7a25c8262d4fe37a6492bbc11e
---

diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34
index 09f5346933..136ecc3306 100644
--- a/RELEASE-NOTES-1.34
+++ b/RELEASE-NOTES-1.34
@@ -164,6 +164,18 @@ $wgPasswordPolicy['policies']['default']['PasswordNotInLargeBlacklist'] = false;
   deprecated in 1.25, has been removed.
 * (T60993) action=query list=filearchive, list=alldeletedrevisions and
   prop=deletedrevisions no longer require the 'deletedhistory' user right.
+* In the response to queries that use 'prop=imageinfo', entries for
+  non-existing files (indicated by the 'filemissing' field) now omit the
+  following fields, since they are meaningless in this context:
+  'timestamp', 'userhidden', 'user', 'userid', 'anon', 'size', 'width',
+  'height', 'pagecount', 'duration', 'commenthidden', 'parsedcomment',
+  'comment', 'thumburl', 'thumbwidth', 'thumbheight', 'thumbmime',
+  'thumberror', 'url', 'sha1', 'metadata', 'extmetadata', 'commonmetadata',
+  'mime', 'mediadtype', 'bitdepth'.
+  Clients that process these fields should first check if 'filemissing' is
+  set. Fields that are supported even if the file is missing include:
+  'canonicaltitle', ''archivename' (deleted files only), 'descriptionurl',
+  'descriptionshorturl'.
 
 === Action API internal changes in 1.34 ===
 * The exception thrown in ApiModuleManager::getModule has been changed
diff --git a/includes/api/ApiQueryImageInfo.php b/includes/api/ApiQueryImageInfo.php
index 97a9b0a5b5..285c0bfcc9 100644
--- a/includes/api/ApiQueryImageInfo.php
+++ b/includes/api/ApiQueryImageInfo.php
@@ -387,9 +387,13 @@ class ApiQueryImageInfo extends ApiQueryBase {
 		$vals = [
 			ApiResult::META_TYPE => 'assoc',
 		];
+
+		// Some information will be unavailable if the file does not exist. T221812
+		$exists = $file->exists();
+
 		// Timestamp is shown even if the file is revdelete'd in interface
 		// so do same here.
-		if ( isset( $prop['timestamp'] ) ) {
+		if ( isset( $prop['timestamp'] ) && $exists ) {
 			$vals['timestamp'] = wfTimestamp( TS_ISO_8601, $file->getTimestamp() );
 		}
 
@@ -408,7 +412,7 @@ class ApiQueryImageInfo extends ApiQueryBase {
 		$user = isset( $prop['user'] );
 		$userid = isset( $prop['userid'] );
 
-		if ( $user || $userid ) {
+		if ( ( $user || $userid ) && $exists ) {
 			if ( $file->isDeleted( File::DELETED_USER ) ) {
 				$vals['userhidden'] = true;
 				$anyHidden = true;
@@ -428,7 +432,7 @@ class ApiQueryImageInfo extends ApiQueryBase {
 
 		// This is shown even if the file is revdelete'd in interface
 		// so do same here.
-		if ( isset( $prop['size'] ) || isset( $prop['dimensions'] ) ) {
+		if ( ( isset( $prop['size'] ) || isset( $prop['dimensions'] ) ) && $exists ) {
 			$vals['size'] = (int)$file->getSize();
 			$vals['width'] = (int)$file->getWidth();
 			$vals['height'] = (int)$file->getHeight();
@@ -449,7 +453,7 @@ class ApiQueryImageInfo extends ApiQueryBase {
 		$pcomment = isset( $prop['parsedcomment'] );
 		$comment = isset( $prop['comment'] );
 
-		if ( $pcomment || $comment ) {
+		if ( ( $pcomment || $comment ) && $exists ) {
 			if ( $file->isDeleted( File::DELETED_COMMENT ) ) {
 				$vals['commenthidden'] = true;
 				$anyHidden = true;
@@ -500,7 +504,7 @@ class ApiQueryImageInfo extends ApiQueryBase {
 		}
 
 		if ( $url ) {
-			if ( $file->exists() ) {
+			if ( $exists ) {
 				if ( !is_null( $thumbParams ) ) {
 					$mto = $file->transform( $thumbParams );
 					self::$transformCount++;
@@ -529,8 +533,6 @@ class ApiQueryImageInfo extends ApiQueryBase {
 					}
 				}
 				$vals['url'] = wfExpandUrl( $file->getFullUrl(), PROTO_CURRENT );
-			} else {
-				$vals['filemissing'] = true;
 			}
 			$vals['descriptionurl'] = wfExpandUrl( $file->getDescriptionUrl(), PROTO_CURRENT );
 
@@ -540,11 +542,15 @@ class ApiQueryImageInfo extends ApiQueryBase {
 			}
 		}
 
-		if ( $sha1 ) {
+		if ( !$exists ) {
+			$vals['filemissing'] = true;
+		}
+
+		if ( $sha1 && $exists ) {
 			$vals['sha1'] = Wikimedia\base_convert( $file->getSha1(), 36, 16, 40 );
 		}
 
-		if ( $meta ) {
+		if ( $meta && $exists ) {
 			Wikimedia\suppressWarnings();
 			$metadata = unserialize( $file->getMetadata() );
 			Wikimedia\restoreWarnings();
@@ -553,12 +559,12 @@ class ApiQueryImageInfo extends ApiQueryBase {
 			}
 			$vals['metadata'] = $metadata ? static::processMetaData( $metadata, $result ) : null;
 		}
-		if ( $commonmeta ) {
+		if ( $commonmeta && $exists ) {
 			$metaArray = $file->getCommonMetaArray();
 			$vals['commonmetadata'] = $metaArray ? static::processMetaData( $metaArray, $result ) : [];
 		}
 
-		if ( $extmetadata ) {
+		if ( $extmetadata && $exists ) {
 			// Note, this should return an array where all the keys
 			// start with a letter, and all the values are strings.
 			// Thus there should be no issue with format=xml.
@@ -575,11 +581,11 @@ class ApiQueryImageInfo extends ApiQueryBase {
 			$vals['extmetadata'] = $extmetaArray;
 		}
 
-		if ( $mime ) {
+		if ( $mime && $exists ) {
 			$vals['mime'] = $file->getMimeType();
 		}
 
-		if ( $mediatype ) {
+		if ( $mediatype && $exists ) {
 			$vals['mediatype'] = $file->getMediaType();
 		}
 
@@ -589,7 +595,7 @@ class ApiQueryImageInfo extends ApiQueryBase {
 			$vals['archivename'] = $file->getArchiveName();
 		}
 
-		if ( $bitdepth ) {
+		if ( $bitdepth && $exists ) {
 			$vals['bitdepth'] = $file->getBitDepth();
 		}
 
diff --git a/includes/filerepo/file/File.php b/includes/filerepo/file/File.php
index 60d4e29dac..73b08e6a34 100644
--- a/includes/filerepo/file/File.php
+++ b/includes/filerepo/file/File.php
@@ -2042,7 +2042,7 @@ abstract class File implements IDBAccessObject {
 	 * Get the URL of the image description page. May return false if it is
 	 * unknown or not applicable.
 	 *
-	 * @return string
+	 * @return string|bool
 	 */
 	function getDescriptionUrl() {
 		if ( $this->repo ) {
diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index ceb8dda9c7..6d2943381f 100644
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -452,6 +452,10 @@ class LocalFile extends File {
 	 * This covers fields that are sometimes not cached.
 	 */
 	protected function loadExtraFromDB() {
+		if ( !$this->title ) {
+			return; // Avoid hard failure when the file does not exist. T221812
+		}
+
 		$fname = static::class . '::' . __FUNCTION__;
 
 		# Unconditionally set loaded=true, we don't want the accessors constantly rechecking
@@ -857,12 +861,24 @@ class LocalFile extends File {
 	function getUser( $type = 'text' ) {
 		$this->load();
 
-		if ( $type === 'object' ) {
-			return $this->user;
-		} elseif ( $type === 'text' ) {
-			return $this->user->getName();
-		} elseif ( $type === 'id' ) {
-			return $this->user->getId();
+		if ( !$this->user ) {
+			// If the file does not exist, $this->user will be null, see T221812.
+			// Note: 'Unknown user' this is a reserved user name.
+			if ( $type === 'object' ) {
+				return User::newFromName( 'Unknown user', false );
+			} elseif ( $type === 'text' ) {
+				return 'Unknown user';
+			} elseif ( $type === 'id' ) {
+				return 0;
+			}
+		} else {
+			if ( $type === 'object' ) {
+				return $this->user;
+			} elseif ( $type === 'text' ) {
+				return $this->user->getName();
+			} elseif ( $type === 'id' ) {
+				return $this->user->getId();
+			}
 		}
 
 		throw new MWException( "Unknown type '$type'." );
@@ -876,9 +892,13 @@ class LocalFile extends File {
 	 * @since 1.27
 	 */
 	public function getDescriptionShortUrl() {
+		if ( !$this->title ) {
+			return null; // Avoid hard failure when the file does not exist. T221812
+		}
+
 		$pageId = $this->title->getArticleID();
 
-		if ( $pageId !== null ) {
+		if ( $pageId ) {
 			$url = $this->repo->makeUrl( [ 'curid' => $pageId ] );
 			if ( $url !== false ) {
 				return $url;
@@ -1145,6 +1165,10 @@ class LocalFile extends File {
 	 * @return OldLocalFile[]
 	 */
 	function getHistory( $limit = null, $start = null, $end = null, $inc = true ) {
+		if ( !$this->exists() ) {
+			return []; // Avoid hard failure when the file does not exist. T221812
+		}
+
 		$dbr = $this->repo->getReplicaDB();
 		$oldFileQuery = OldLocalFile::getQueryInfo();
 
@@ -1198,9 +1222,13 @@ class LocalFile extends File {
 	 *  0      return line for current version
 	 *  1      query for old versions, return first one
 	 *  2, ... return next old version from above query
-	 * @return bool
+	 * @return stdClass|bool
 	 */
 	public function nextHistoryLine() {
+		if ( !$this->exists() ) {
+			return false; // Avoid hard failure when the file does not exist. T221812
+		}
+
 		# Polymorphic function name to distinguish foreign and local fetches
 		$fname = static::class . '::' . __FUNCTION__;
 
@@ -2026,9 +2054,13 @@ class LocalFile extends File {
 
 	/**
 	 * Get the URL of the file description page.
-	 * @return string
+	 * @return string|bool
 	 */
 	function getDescriptionUrl() {
+		if ( !$this->title ) {
+			return false; // Avoid hard failure when the file does not exist. T221812
+		}
+
 		return $this->title->getLocalURL();
 	}
 
@@ -2041,6 +2073,10 @@ class LocalFile extends File {
 	 * @return string|false
 	 */
 	function getDescriptionText( Language $lang = null ) {
+		if ( !$this->title ) {
+			return false; // Avoid hard failure when the file does not exist. T221812
+		}
+
 		$store = MediaWikiServices::getInstance()->getRevisionStore();
 		$revision = $store->getRevisionByTitle( $this->title, 0, Revision::READ_NORMAL );
 		if ( !$revision ) {
@@ -2090,6 +2126,10 @@ class LocalFile extends File {
 	 * @return bool|string
 	 */
 	public function getDescriptionTouched() {
+		if ( !$this->exists() ) {
+			return false; // Avoid hard failure when the file does not exist. T221812
+		}
+
 		// The DB lookup might return false, e.g. if the file was just deleted, or the shared DB repo
 		// itself gets it from elsewhere. To avoid repeating the DB lookups in such a case, we
 		// need to differentiate between null (uninitialized) and false (failed to load).
diff --git a/tests/phpunit/includes/filerepo/file/LocalFileTest.php b/tests/phpunit/includes/filerepo/file/LocalFileTest.php
index 7acd237203..8f378052a3 100644
--- a/tests/phpunit/includes/filerepo/file/LocalFileTest.php
+++ b/tests/phpunit/includes/filerepo/file/LocalFileTest.php
@@ -193,4 +193,26 @@ class LocalFileTest extends MediaWikiTestCase {
 			'wfLocalFile() returns LocalFile for valid Titles'
 		);
 	}
+
+	/**
+	 * @covers File::getUser
+	 */
+	public function testGetUserForNonExistingFile() {
+		$this->assertSame( 'Unknown user', $this->file_hl0->getUser() );
+		$this->assertSame( 0, $this->file_hl0->getUser( 'id' ) );
+	}
+
+	/**
+	 * @covers File::getUser
+	 */
+	public function testDescriptionShortUrlForNonExistingFile() {
+		$this->assertNull( $this->file_hl0->getDescriptionShortUrl() );
+	}
+
+	/**
+	 * @covers File::getUser
+	 */
+	public function testDescriptionTextForNonExistingFile() {
+		$this->assertFalse( $this->file_hl0->getDescriptionText() );
+	}
 }