FSFile and TempFSFile cleanups
authorAaron Schulz <aschulz@wikimedia.org>
Mon, 19 Sep 2016 00:22:36 +0000 (17:22 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Mon, 19 Sep 2016 19:55:09 +0000 (19:55 +0000)
* Remove wf* function dependencies. This includes wfTempDir().
  Callers now should specify the directory, though it will try to do
  most of the wfTempDir() logic anyway if they do not.
* Update callers to inject wfTempDir() so $wgTmpDirectory is used by
  TempFSFile instead of it probing to find a valid directory itself.
* Move most of the wfTempDir() logic to TempFSFile::getUsableTempDirectory().
* Remove unused getMimeType() method.

Change-Id: Idd55936b07f9448a6c90577708722b7b52b8fe66

includes/GlobalFunctions.php
includes/api/ApiImageRotate.php
includes/filebackend/FSFile.php
includes/filebackend/TempFSFile.php
includes/filerepo/file/File.php
includes/upload/UploadFromChunks.php
includes/upload/UploadFromUrl.php
tests/phpunit/includes/filebackend/FileBackendTest.php
tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php
tests/phpunit/mocks/filebackend/MockFSFile.php

index 90bba53..e5f518f 100644 (file)
@@ -2078,35 +2078,7 @@ function wfTempDir() {
                return $wgTmpDirectory;
        }
 
-       $tmpDir = array_map( "getenv", [ 'TMPDIR', 'TMP', 'TEMP' ] );
-       $tmpDir[] = sys_get_temp_dir();
-       $tmpDir[] = ini_get( 'upload_tmp_dir' );
-
-       foreach ( $tmpDir as $tmp ) {
-               if ( $tmp && file_exists( $tmp ) && is_dir( $tmp ) && is_writable( $tmp ) ) {
-                       return $tmp;
-               }
-       }
-
-       /**
-        * PHP on Windows will detect C:\Windows\Temp as not writable even though PHP can write to it
-        * so create a directory within that called 'mwtmp' with a suffix of the user running the
-        * current process.
-        * The user is included as if various scripts are run by different users they will likely
-        * not be able to access each others temporary files.
-        */
-       if ( wfIsWindows() ) {
-               $tmp = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'mwtmp' . '-' . get_current_user();
-               if ( !file_exists( $tmp ) ) {
-                       mkdir( $tmp );
-               }
-               if ( file_exists( $tmp ) && is_dir( $tmp ) && is_writable( $tmp ) ) {
-                       return $tmp;
-               }
-       }
-
-       throw new MWException( 'No writable temporary directory could be found. ' .
-               'Please set $wgTmpDirectory to a writable directory.' );
+       return TempFSFile::getUsableTempDirectory();
 }
 
 /**
index 2b99353..966bcbf 100644 (file)
@@ -108,7 +108,7 @@ class ApiImageRotate extends ApiBase {
                                continue;
                        }
                        $ext = strtolower( pathinfo( "$srcPath", PATHINFO_EXTENSION ) );
-                       $tmpFile = TempFSFile::factory( 'rotate_', $ext );
+                       $tmpFile = TempFSFile::factory( 'rotate_', $ext, wfTempDir() );
                        $dstPath = $tmpFile->getPath();
                        $err = $handler->rotate( $file, [
                                'srcPath' => $srcPath,
index 8aa11b6..221fe9f 100644 (file)
@@ -85,15 +85,6 @@ class FSFile {
                return $timestamp;
        }
 
-       /**
-        * Guess the MIME type from the file contents alone
-        *
-        * @return string
-        */
-       public function getMimeType() {
-               return MimeMagic::singleton()->guessMimeType( $this->path, false );
-       }
-
        /**
         * Get an associative array containing information about
         * a file with the given storage path.
@@ -117,8 +108,6 @@ class FSFile {
         * @return array
         */
        public function getProps( $ext = true ) {
-               wfDebug( __METHOD__ . ": Getting file info for $this->path\n" );
-
                $info = self::placeholderProps();
                $info['fileExists'] = $this->exists();
 
@@ -131,7 +120,7 @@ class FSFile {
                        }
 
                        # MIME type according to file contents
-                       $info['file-mime'] = $this->getMimeType();
+                       $info['file-mime'] = $magic->guessMimeType( $this->path, false );
                        # logical MIME type
                        $info['mime'] = $magic->improveTypeFromExtension( $info['file-mime'], $ext );
 
@@ -145,17 +134,15 @@ class FSFile {
                        $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'] );
                                if ( is_array( $gis ) ) {
                                        $info = $this->extractImageSizeInfo( $gis ) + $info;
                                }
                        }
                        $info['sha1'] = $this->getSha1Base36();
-
-                       wfDebug( __METHOD__ . ": $this->path loaded, {$info['size']} bytes, {$info['mime']}.\n" );
-               } else {
-                       wfDebug( __METHOD__ . ": $this->path NOT FOUND!\n" );
                }
 
                return $info;
index f572840..fed6812 100644 (file)
@@ -48,15 +48,20 @@ class TempFSFile extends FSFile {
         * Temporary files may be purged when the file object falls out of scope.
         *
         * @param string $prefix
-        * @param string $extension
+        * @param string $extension Optional file extension
+        * @param string|null $tmpDirectory Optional parent directory
         * @return TempFSFile|null
         */
-       public static function factory( $prefix, $extension = '' ) {
+       public static function factory( $prefix, $extension = '', $tmpDirectory = null ) {
                $ext = ( $extension != '' ) ? ".{$extension}" : '';
 
                $attempts = 5;
                while ( $attempts-- ) {
-                       $path = wfTempDir() . '/' . $prefix . wfRandomString( 12 ) . $ext;
+                       $hex = sprintf( '%06x%06x', mt_rand( 0, 0xffffff ), mt_rand( 0, 0xffffff ) );
+                       if ( !is_string( $tmpDirectory ) ) {
+                               $tmpDirectory = self::getUsableTempDirectory();
+                       }
+                       $path = wfTempDir() . '/' . $prefix . $hex . $ext;
                        MediaWiki\suppressWarnings();
                        $newFileHandle = fopen( $path, 'x' );
                        MediaWiki\restoreWarnings();
@@ -73,6 +78,40 @@ class TempFSFile extends FSFile {
                return null;
        }
 
+       /**
+        * @return string Filesystem path to a temporary directory
+        * @throws RuntimeException
+        */
+       public static function getUsableTempDirectory() {
+               $tmpDir = array_map( 'getenv', [ 'TMPDIR', 'TMP', 'TEMP' ] );
+               $tmpDir[] = sys_get_temp_dir();
+               $tmpDir[] = ini_get( 'upload_tmp_dir' );
+               foreach ( $tmpDir as $tmp ) {
+                       if ( $tmp != '' && is_dir( $tmp ) && is_writable( $tmp ) ) {
+                               return $tmp;
+                       }
+               }
+
+               // PHP on Windows will detect C:\Windows\Temp as not writable even though PHP can write to
+               // it so create a directory within that called 'mwtmp' with a suffix of the user running
+               // the current process.
+               // The user is included as if various scripts are run by different users they will likely
+               // not be able to access each others temporary files.
+               if ( strtoupper( substr( PHP_OS, 0, 3 ) ) === 'WIN' ) {
+                       $tmp = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'mwtmp-' . get_current_user();
+                       if ( !file_exists( $tmp ) ) {
+                               mkdir( $tmp );
+                       }
+                       if ( is_dir( $tmp ) && is_writable( $tmp ) ) {
+                               return $tmp;
+                       }
+               }
+
+               throw new RuntimeException(
+                       'No writable temporary directory could be found. ' .
+                       'Please explicitly specify a writable directory in configuration.' );
+       }
+
        /**
         * Purge this file off the file system
         *
index 425a08c..c48866b 100644 (file)
@@ -1328,7 +1328,7 @@ abstract class File implements IDBAccessObject {
         */
        protected function makeTransformTmpFile( $thumbPath ) {
                $thumbExt = FileBackend::extensionFromPath( $thumbPath );
-               return TempFSFile::factory( 'transform_', $thumbExt );
+               return TempFSFile::factory( 'transform_', $thumbExt, wfTempDir() );
        }
 
        /**
index 9145a85..08cf434 100644 (file)
@@ -130,7 +130,7 @@ class UploadFromChunks extends UploadFromFile {
                // Get the file extension from the last chunk
                $ext = FileBackend::extensionFromPath( $this->mVirtualTempPath );
                // Get a 0-byte temp file to perform the concatenation at
-               $tmpFile = TempFSFile::factory( 'chunkedupload_', $ext );
+               $tmpFile = TempFSFile::factory( 'chunkedupload_', $ext, wfTempDir() );
                $tmpPath = false; // fail in concatenate()
                if ( $tmpFile ) {
                        // keep alive with $this
index 6639c34..865f630 100644 (file)
@@ -202,7 +202,7 @@ class UploadFromUrl extends UploadBase {
         * @return string Path to the file
         */
        protected function makeTemporaryFile() {
-               $tmpFile = TempFSFile::factory( 'URL' );
+               $tmpFile = TempFSFile::factory( 'URL', 'urlupload_', wfTempDir() );
                $tmpFile->bind( $this );
 
                return $tmpFile->getPath();
index 254cfbd..ca562aa 100644 (file)
@@ -268,7 +268,7 @@ class FileBackendTest extends MediaWikiTestCase {
        public static function provider_testStore() {
                $cases = [];
 
-               $tmpName = TempFSFile::factory( "unittests_", 'txt' )->getPath();
+               $tmpName = TempFSFile::factory( "unittests_", 'txt', wfTempDir() )->getPath();
                $toPath = self::baseStorePath() . '/unittest-cont1/e/fun/obj1.txt';
                $op = [ 'op' => 'store', 'src' => $tmpName, 'dst' => $toPath ];
                $cases[] = [ $op ];
@@ -1786,9 +1786,9 @@ class FileBackendTest extends MediaWikiTestCase {
                $fileBContents = 'g-jmq3gpqgt3qtg q3GT ';
                $fileCContents = 'eigna[ogmewt 3qt g3qg flew[ag';
 
-               $tmpNameA = TempFSFile::factory( "unittests_", 'txt' )->getPath();
-               $tmpNameB = TempFSFile::factory( "unittests_", 'txt' )->getPath();
-               $tmpNameC = TempFSFile::factory( "unittests_", 'txt' )->getPath();
+               $tmpNameA = TempFSFile::factory( "unittests_", 'txt', wfTempDir() )->getPath();
+               $tmpNameB = TempFSFile::factory( "unittests_", 'txt', wfTempDir() )->getPath();
+               $tmpNameC = TempFSFile::factory( "unittests_", 'txt', wfTempDir() )->getPath();
                $this->addTmpFiles( [ $tmpNameA, $tmpNameB, $tmpNameC ] );
                file_put_contents( $tmpNameA, $fileAContents );
                file_put_contents( $tmpNameB, $fileBContents );
index ed80c57..92a54fa 100644 (file)
@@ -59,7 +59,8 @@ class MigrateFileRepoLayoutTest extends MediaWikiTestCase {
                        ->method( 'getRepo' )
                        ->will( $this->returnValue( $repoMock ) );
 
-               $this->tmpFilepath = TempFSFile::factory( 'migratefilelayout-test-', 'png' )->getPath();
+               $this->tmpFilepath = TempFSFile::factory(
+                       'migratefilelayout-test-', 'png', wfTempDir() )->getPath();
 
                file_put_contents( $this->tmpFilepath, $this->text );
 
index bdeed58..6797f59 100644 (file)
@@ -50,15 +50,11 @@ class MockFSFile extends FSFile {
                return wfTimestamp( TS_MW );
        }
 
-       public function getMimeType() {
-               return 'text/mock';
-       }
-
        public function getProps( $ext = true ) {
                return [
                        'fileExists' => $this->exists(),
                        'size' => $this->getSize(),
-                       'file-mime' => $this->getMimeType(),
+                       'file-mime' => 'text/mock',
                        'sha1' => $this->getSha1Base36(),
                ];
        }