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 09f5346..136ecc3 100644 (file)
--- 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 97a9b0a..285c0bf 100644 (file)
--- 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 60d4e29..73b08e6 100644 (file)
--- 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 ceb8dda..6d29433 100644 (file)
--- 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 7acd237..8f37805 100644 (file)
--- 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() );
+       }
 }