Migrate callers to new MWFileProps::getPropsFromPath() method
authorAaron Schulz <aschulz@wikimedia.org>
Mon, 19 Sep 2016 01:39:59 +0000 (18:39 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 22 Sep 2016 00:33:46 +0000 (00:33 +0000)
* FSFile should not be responsible for handling this much logic.
* Make more MediaHandler classes aware of the fact that an object
  other than File might be passed in. Use the FSFile instead of a
  useless empty stdClass object.
* Also added more fields to FSFile::placeholderProps to make it
  more complete.

Change-Id: I9fe764b2a7261af507c6555e6a57273cf7d00d36

24 files changed:
autoload.php
includes/MimeMagic.php
includes/filebackend/FSFile.php
includes/filerepo/FileRepo.php
includes/filerepo/RepoGroup.php
includes/filerepo/file/LocalFile.php
includes/media/BMP.php
includes/media/DjVu.php
includes/media/ExifBitmap.php
includes/media/MediaHandler.php
includes/media/PNG.php
includes/media/SVG.php
includes/media/Tiff.php
includes/media/WebP.php
includes/media/XCF.php
includes/upload/UploadBase.php
includes/upload/UploadStash.php
includes/utils/MWFileProps.php [new file with mode: 0644]
maintenance/importImages.php
tests/common/TestsAutoLoader.php
tests/parser/ParserTestRunner.php
tests/phpunit/MediaWikiTestCase.php
tests/phpunit/includes/media/MediaWikiMediaTestCase.php
tests/phpunit/mocks/filerepo/MockLocalRepo.php [new file with mode: 0644]

index 0c16e76..f5c334b 100644 (file)
@@ -773,6 +773,7 @@ $wgAutoloadLocalClasses = [
        'MWException' => __DIR__ . '/includes/exception/MWException.php',
        'MWExceptionHandler' => __DIR__ . '/includes/exception/MWExceptionHandler.php',
        'MWExceptionRenderer' => __DIR__ . '/includes/exception/MWExceptionRenderer.php',
+       'MWFileProps' => __DIR__ . '/includes/utils/MWFileProps.php',
        'MWGrants' => __DIR__ . '/includes/utils/MWGrants.php',
        'MWHttpRequest' => __DIR__ . '/includes/HttpFunctions.php',
        'MWMemcached' => __DIR__ . '/includes/compat/MemcachedClientCompat.php',
index a432d44..5551865 100644 (file)
@@ -1046,6 +1046,7 @@ class MimeMagic {
                        }
                }
 
+               $type = null;
                // Check for entry for full MIME type
                if ( $mime ) {
                        $type = $this->findMediaType( $mime );
index 221fe9f..e7e2608 100644 (file)
@@ -112,37 +112,30 @@ class FSFile {
                $info['fileExists'] = $this->exists();
 
                if ( $info['fileExists'] ) {
+                       $info['size'] = $this->getSize(); // bytes
+                       $info['sha1'] = $this->getSha1Base36();
+                       // @TODO: replace the code below with bare FileInfo use so this can go in /libs
                        $magic = MimeMagic::singleton();
 
-                       # get the file extension
-                       if ( $ext === true ) {
-                               $ext = self::extensionFromPath( $this->path );
-                       }
-
                        # MIME type according to file contents
                        $info['file-mime'] = $magic->guessMimeType( $this->path, false );
-                       # logical MIME type
+                       # Logical MIME type
+                       $ext = ( $ext === true ) ? FileBackend::extensionFromPath( $this->path ) : $ext;
                        $info['mime'] = $magic->improveTypeFromExtension( $info['file-mime'], $ext );
 
                        list( $info['major_mime'], $info['minor_mime'] ) = File::splitMime( $info['mime'] );
                        $info['media_type'] = $magic->getMediaType( $this->path, $info['mime'] );
 
-                       # Get size in bytes
-                       $info['size'] = $this->getSize();
-
                        # Height, width and metadata
                        $handler = MediaHandler::getHandler( $info['mime'] );
                        if ( $handler ) {
-                               $tempImage = (object)[]; // XXX (hack for File object)
-                               /** @noinspection PhpParamsInspection */
-                               $info['metadata'] = $handler->getMetadata( $tempImage, $this->path );
-                               /** @noinspection PhpParamsInspection */
-                               $gis = $handler->getImageSize( $tempImage, $this->path, $info['metadata'] );
+                               $info['metadata'] = $handler->getMetadata( $this, $this->path );
+                               /** @noinspection PhpMethodParametersCountMismatchInspection */
+                               $gis = $handler->getImageSize( $this, $this->path, $info['metadata'] );
                                if ( is_array( $gis ) ) {
                                        $info = $this->extractImageSizeInfo( $gis ) + $info;
                                }
                        }
-                       $info['sha1'] = $this->getSha1Base36();
                }
 
                return $info;
@@ -153,7 +146,11 @@ class FSFile {
         *
         * Resulting array fields include:
         *   - fileExists
+        *   - size
+        *   - file-mime (as major/minor)
         *   - mime (as major/minor)
+        *   - major_mime
+        *   - minor_mime
         *   - media_type (value to be used with the MEDIATYPE_xxx constants)
         *   - metadata (handler specific)
         *   - sha1 (in base 36)
@@ -166,6 +163,10 @@ class FSFile {
        public static function placeholderProps() {
                $info = [];
                $info['fileExists'] = false;
+               $info['size'] = 0;
+               $info['file-mime'] = null;
+               $info['major_mime'] = null;
+               $info['minor_mime'] = null;
                $info['mime'] = null;
                $info['media_type'] = MEDIATYPE_UNKNOWN;
                $info['metadata'] = '';
index 8fee3bf..b5c5bf3 100644 (file)
@@ -1539,9 +1539,15 @@ class FileRepo {
         * @return array
         */
        public function getFileProps( $virtualUrl ) {
-               $path = $this->resolveToStoragePath( $virtualUrl );
+               $fsFile = $this->getLocalReference( $virtualUrl );
+               if ( $fsFile ) {
+                       $mwProps = new MWFileProps( MimeMagic::singleton() );
+                       $props = $mwProps->getPropsFromPath( $fsFile->getPath(), true );
+               } else {
+                       $props = FSFile::placeholderProps();
+               }
 
-               return $this->backend->getFileProps( [ 'src' => $path ] );
+               return $props;
        }
 
        /**
index bd32de0..d47624f 100644 (file)
@@ -452,7 +452,9 @@ class RepoGroup {
 
                        return $repo->getFileProps( $fileName );
                } else {
-                       return FSFile::getPropsFromPath( $fileName );
+                       $mwProps = new MWFileProps( MimeMagic::singleton() );
+
+                       return $mwProps->getPropsFromPath( $fileName, true );
                }
        }
 
index 396b47c..60cfdac 100644 (file)
@@ -1179,7 +1179,8 @@ class LocalFile extends File {
                        ) {
                                $props = $this->repo->getFileProps( $srcPath );
                        } else {
-                               $props = FSFile::getPropsFromPath( $srcPath );
+                               $mwProps = new MWFileProps( MimeMagic::singleton() );
+                               $props = $mwProps->getPropsFromPath( $srcPath, true );
                        }
                }
 
index 4b9b268..0229ac1 100644 (file)
@@ -51,7 +51,7 @@ class BmpHandler extends BitmapHandler {
        /**
         * Get width and height from the bmp header.
         *
-        * @param File $image
+        * @param File|FSFile $image
         * @param string $filename
         * @return array
         */
index 9add138..18f75ec 100644 (file)
@@ -235,7 +235,7 @@ class DjVuHandler extends ImageHandler {
        /**
         * Cache an instance of DjVuImage in an Image object, return that instance
         *
-        * @param File $image
+        * @param File|FSFile $image
         * @param string $path
         * @return DjVuImage
         */
@@ -335,11 +335,6 @@ class DjVuHandler extends ImageHandler {
                }
        }
 
-       /**
-        * @param File $image
-        * @param string $path
-        * @return bool|array False on failure
-        */
        function getImageSize( $image, $path ) {
                return $this->getDjVuImage( $image, $path )->getImageSize();
        }
index 732be3d..7aeefa0 100644 (file)
@@ -165,7 +165,7 @@ class ExifBitmapHandler extends BitmapHandler {
         * Wrapper for base classes ImageHandler::getImageSize() that checks for
         * rotation reported from metadata and swaps the sizes to match.
         *
-        * @param File $image
+        * @param File|FSFile $image
         * @param string $path
         * @return array
         */
index 70a43f2..4ca2663 100644 (file)
@@ -100,21 +100,22 @@ abstract class MediaHandler {
         * @note If this is a multipage file, return the width and height of the
         *  first page.
         *
-        * @param File $image The image object, or false if there isn't one
+        * @param File|FSFile $image The image object, or false if there isn't one.
+        *   Warning, FSFile::getPropsFromPath might pass an FSFile instead of File (!)
         * @param string $path The filename
-        * @return array Follow the format of PHP getimagesize() internal function.
+        * @return array|bool Follow the format of PHP getimagesize() internal function.
         *   See http://www.php.net/getimagesize. MediaWiki will only ever use the
         *   first two array keys (the width and height), and the 'bits' associative
         *   key. All other array keys are ignored. Returning a 'bits' key is optional
-        *   as not all formats have a notion of "bitdepth".
+        *   as not all formats have a notion of "bitdepth". Returns false on failure.
         */
        abstract function getImageSize( $image, $path );
 
        /**
         * Get handler-specific metadata which will be saved in the img_metadata field.
         *
-        * @param File $image The image object, or false if there isn't one.
-        *   Warning, FSFile::getPropsFromPath might pass an (object)array() instead (!)
+        * @param File|FSFile $image The image object, or false if there isn't one.
+        *   Warning, FSFile::getPropsFromPath might pass an FSFile instead of File (!)
         * @param string $path The filename
         * @return string A string of metadata in php serialized form (Run through serialize())
         */
index 8a3e001..294abb3 100644 (file)
@@ -30,7 +30,7 @@ class PNGHandler extends BitmapHandler {
        const BROKEN_FILE = '0';
 
        /**
-        * @param File $image
+        * @param File|FSFile $image
         * @param string $filename
         * @return string
         */
index 2bb6d13..8360920 100644 (file)
@@ -301,13 +301,13 @@ class SvgHandler extends ImageHandler {
        }
 
        /**
-        * @param File $file
+        * @param File|FSFile $file
         * @param string $path Unused
         * @param bool|array $metadata
         * @return array
         */
        function getImageSize( $file, $path, $metadata = false ) {
-               if ( $metadata === false ) {
+               if ( $metadata === false && $file instanceof File ) {
                        $metadata = $file->getMetadata();
                }
                $metadata = $this->unpackMetadata( $metadata );
@@ -355,7 +355,7 @@ class SvgHandler extends ImageHandler {
        }
 
        /**
-        * @param File $file
+        * @param File|FSFile $file
         * @param string $filename
         * @return string Serialised metadata
         */
index 2e73249..f0f4cda 100644 (file)
@@ -71,13 +71,14 @@ class TiffHandler extends ExifBitmapHandler {
        }
 
        /**
-        * @param File $image
+        * @param File|FSFile $image
         * @param string $filename
         * @throws MWException
         * @return string
         */
        function getMetadata( $image, $filename ) {
                global $wgShowEXIF;
+
                if ( $wgShowEXIF ) {
                        try {
                                $meta = BitmapMetadataHandler::Tiff( $filename );
index 35e885f..e2c2d2d 100644 (file)
@@ -230,7 +230,7 @@ class WebPHandler extends BitmapHandler {
                if ( $file === null ) {
                        $metadata = self::getMetadata( $file, $path );
                }
-               if ( $metadata === false ) {
+               if ( $metadata === false && $file instanceof File ) {
                        $metadata = $file->getMetadata();
                }
 
index 6ac675e..108d6fb 100644 (file)
@@ -56,7 +56,7 @@ class XCFHandler extends BitmapHandler {
        /**
         * Get width and height from the XCF header.
         *
-        * @param File $image
+        * @param File|FSFile $image
         * @param string $filename
         * @return array
         */
@@ -149,7 +149,7 @@ class XCFHandler extends BitmapHandler {
         *
         * Greyscale files need different command line options.
         *
-        * @param File $file The image object, or false if there isn't one.
+        * @param File|FSFile $file The image object, or false if there isn't one.
         *   Warning, FSFile::getPropsFromPath might pass an (object)array() instead (!)
         * @param string $filename The filename
         * @return string
index 1be5c24..91b4133 100644 (file)
@@ -446,7 +446,8 @@ abstract class UploadBase {
                        return $status;
                }
 
-               $this->mFileProps = FSFile::getPropsFromPath( $this->mTempPath, $this->mFinalExtension );
+               $mwProps = new MWFileProps( MimeMagic::singleton() );
+               $this->mFileProps = $mwProps->getPropsFromPath( $this->mTempPath, $this->mFinalExtension );
                $mime = $this->mFileProps['mime'];
 
                if ( $wgVerifyMimeType ) {
@@ -504,7 +505,8 @@ abstract class UploadBase {
                # getTitle() sets some internal parameters like $this->mFinalExtension
                $this->getTitle();
 
-               $this->mFileProps = FSFile::getPropsFromPath( $this->mTempPath, $this->mFinalExtension );
+               $mwProps = new MWFileProps( MimeMagic::singleton() );
+               $this->mFileProps = $mwProps->getPropsFromPath( $this->mTempPath, $this->mFinalExtension );
 
                # check MIME type, if desired
                $mime = $this->mFileProps['file-mime'];
index 000c6a4..b7160b3 100644 (file)
@@ -207,7 +207,9 @@ class UploadStash {
                        wfDebug( __METHOD__ . " tried to stash file at '$path', but it doesn't exist\n" );
                        throw new UploadStashBadPathException( "path doesn't exist" );
                }
-               $fileProps = FSFile::getPropsFromPath( $path );
+
+               $mwProps = new MWFileProps( MimeMagic::singleton() );
+               $fileProps = $mwProps->getPropsFromPath( $path, true );
                wfDebug( __METHOD__ . " stashing file at '$path'\n" );
 
                // we will be initializing from some tmpnam files that don't have extensions.
diff --git a/includes/utils/MWFileProps.php b/includes/utils/MWFileProps.php
new file mode 100644 (file)
index 0000000..b8ecd4c
--- /dev/null
@@ -0,0 +1,115 @@
+<?php
+/**
+ * MimeMagic helper functions for detecting and dealing with MIME types.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ */
+
+/**
+ * MimeMagic helper wrapper
+ *
+ * @since 1.28
+ */
+class MWFileProps {
+       /** @var MimeMagic */
+       private $magic;
+
+       /**
+        * @param MimeMagic $magic
+        */
+       public function __construct( MimeMagic $magic ) {
+               $this->magic = $magic;
+       }
+
+       /**
+        * Get an associative array containing information about
+        * a file with the given storage path.
+        *
+        * Resulting array fields include:
+        *   - fileExists
+        *   - size (filesize in bytes)
+        *   - mime (as major/minor)
+        *   - media_type (value to be used with the MEDIATYPE_xxx constants)
+        *   - metadata (handler specific)
+        *   - sha1 (in base 36)
+        *   - width
+        *   - height
+        *   - bits (bitrate)
+        *   - file-mime
+        *   - major_mime
+        *   - minor_mime
+        *
+        * @param string $path Filesystem path to a file
+        * @param string|bool $ext The file extension, or true to extract it from the filename.
+        *             Set it to false to ignore the extension.
+        * @return array
+        * @since 1.28
+        */
+       public function getPropsFromPath( $path, $ext ) {
+               $fsFile = new FSFile( $path );
+
+               $info = FSFile::placeholderProps();
+               $info['fileExists'] = $fsFile->exists();
+               if ( $info['fileExists'] ) {
+                       $info['size'] = $fsFile->getSize(); // bytes
+                       $info['sha1'] = $fsFile->getSha1Base36();
+
+                       # MIME type according to file contents
+                       $info['file-mime'] = $this->magic->guessMimeType( $path, false );
+                       # Logical MIME type
+                       $ext = ( $ext === true ) ? FileBackend::extensionFromPath( $path ) : $ext;
+                       $info['mime'] = $this->magic->improveTypeFromExtension( $info['file-mime'], $ext );
+
+                       list( $info['major_mime'], $info['minor_mime'] ) = File::splitMime( $info['mime'] );
+                       $info['media_type'] = $this->magic->getMediaType( $path, $info['mime'] );
+
+                       # Height, width and metadata
+                       $handler = MediaHandler::getHandler( $info['mime'] );
+                       if ( $handler ) {
+                               $info['metadata'] = $handler->getMetadata( $fsFile, $path );
+                               /** @noinspection PhpMethodParametersCountMismatchInspection */
+                               $gis = $handler->getImageSize( $fsFile, $path, $info['metadata'] );
+                               if ( is_array( $gis ) ) {
+                                       $info = $this->extractImageSizeInfo( $gis ) + $info;
+                               }
+                       }
+               }
+
+               return $info;
+       }
+
+       /**
+        * Exract image size information
+        *
+        * @param array $gis
+        * @return array
+        */
+       private function extractImageSizeInfo( array $gis ) {
+               $info = [];
+               # NOTE: $gis[2] contains a code for the image type. This is no longer used.
+               $info['width'] = $gis[0];
+               $info['height'] = $gis[1];
+               if ( isset( $gis['bits'] ) ) {
+                       $info['bits'] = $gis['bits'];
+               } else {
+                       $info['bits'] = 0;
+               }
+
+               return $info;
+       }
+}
index ac700ef..5a4ab39 100644 (file)
@@ -245,7 +245,8 @@ if ( $count > 0 ) {
                if ( isset( $options['dry'] ) ) {
                        echo " publishing {$file} by '" . $wgUser->getName() . "', comment '$commentText'... ";
                } else {
-                       $props = FSFile::getPropsFromPath( $file );
+                       $mwProps = new MWFileProps( MimeMagic::singleton() );
+                       $props = $mwProps->getPropsFromPath( $file, true );
                        $flags = 0;
                        $publishOptions = [];
                        $handler = MediaHandler::getHandler( $props['mime'] );
index 2a985fe..a19fea1 100644 (file)
@@ -147,6 +147,7 @@ $wgAutoloadClasses += [
        # tests/phpunit/mocks
        'MockFSFile' => "$testDir/phpunit/mocks/filebackend/MockFSFile.php",
        'MockFileBackend' => "$testDir/phpunit/mocks/filebackend/MockFileBackend.php",
+       'MockLocalRepo' => "$testDir/phpunit/mocks/filerepo/MockLocalRepo.php",
        'MockBitmapHandler' => "$testDir/phpunit/mocks/media/MockBitmapHandler.php",
        'MockImageHandler' => "$testDir/phpunit/mocks/media/MockImageHandler.php",
        'MockSvgHandler' => "$testDir/phpunit/mocks/media/MockSvgHandler.php",
index 4ef778d..d2968a1 100644 (file)
@@ -348,7 +348,8 @@ class ParserTestRunner {
                        $backend = new FSFileBackend( [
                                'name' => 'local-backend',
                                'wikiId' => wfWikiID(),
-                               'basePath' => $this->uploadDir
+                               'basePath' => $this->uploadDir,
+                               'tmpDirectory' => wfTempDir()
                        ] );
                } elseif ( $this->fileBackendName ) {
                        global $wgFileBackends;
@@ -379,7 +380,7 @@ class ParserTestRunner {
 
                return new RepoGroup(
                        [
-                               'class' => 'LocalRepo',
+                               'class' => 'MockLocalRepo',
                                'name' => 'local',
                                'url' => 'http://example.com/images',
                                'hashLevels' => 2,
index cfeb44f..43577ca 100644 (file)
@@ -56,7 +56,7 @@ abstract class MediaWikiTestCase extends PHPUnit_Framework_TestCase {
        private static $useTemporaryTables = true;
        private static $reuseDB = false;
        private static $dbSetup = false;
-       private static $oldTablePrefix = false;
+       private static $oldTablePrefix = '';
 
        /**
         * Original value of PHP's error_reporting setting.
index 5042121..e854ab5 100644 (file)
@@ -26,7 +26,8 @@ abstract class MediaWikiMediaTestCase extends MediaWikiTestCase {
                $this->backend = new FSFileBackend( [
                        'name' => 'localtesting',
                        'wikiId' => wfWikiID(),
-                       'containerPaths' => $containers
+                       'containerPaths' => $containers,
+                       'tmpDirectory' => $this->getNewTempDirectory()
                ] );
                $this->repo = new FSRepo( $this->getRepoOptions() );
        }
diff --git a/tests/phpunit/mocks/filerepo/MockLocalRepo.php b/tests/phpunit/mocks/filerepo/MockLocalRepo.php
new file mode 100644 (file)
index 0000000..eeaf05a
--- /dev/null
@@ -0,0 +1,23 @@
+<?php
+
+/**
+ * Class simulating a local file repo.
+ *
+ * @ingroup FileRepo
+ * @since 1.28
+ */
+class MockLocalRepo extends LocalRepo {
+       function getLocalCopy( $virtualUrl ) {
+               return new MockFSFile( wfTempDir() . '/' . wfRandomString( 32 ) );
+       }
+
+       function getLocalReference( $virtualUrl ) {
+               return new MockFSFile( wfTempDir() . '/' . wfRandomString( 32 ) );
+       }
+
+       function getFileProps( $virtualUrl ) {
+               $fsFile = $this->getLocalReference( $virtualUrl );
+
+               return $fsFile->getProps();
+       }
+}