jenkins-bot has submitted this change and it was merged.

Change subject: FSFile and TempFSFile cleanups
......................................................................


FSFile and TempFSFile cleanups

* 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
---
M includes/GlobalFunctions.php
M includes/api/ApiImageRotate.php
M includes/filebackend/FSFile.php
M includes/filebackend/TempFSFile.php
M includes/filerepo/file/File.php
M includes/upload/UploadFromChunks.php
M includes/upload/UploadFromUrl.php
M tests/phpunit/includes/filebackend/FileBackendTest.php
M tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php
M tests/phpunit/mocks/filebackend/MockFSFile.php
10 files changed, 57 insertions(+), 62 deletions(-)

Approvals:
  Catrope: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index 90bba53..e5f518f 100644
--- a/includes/GlobalFunctions.php
+++ b/includes/GlobalFunctions.php
@@ -2078,35 +2078,7 @@
                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();
 }
 
 /**
diff --git a/includes/api/ApiImageRotate.php b/includes/api/ApiImageRotate.php
index 2b99353..966bcbf 100644
--- a/includes/api/ApiImageRotate.php
+++ b/includes/api/ApiImageRotate.php
@@ -108,7 +108,7 @@
                                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,
diff --git a/includes/filebackend/FSFile.php b/includes/filebackend/FSFile.php
index 8aa11b6..221fe9f 100644
--- a/includes/filebackend/FSFile.php
+++ b/includes/filebackend/FSFile.php
@@ -86,15 +86,6 @@
        }
 
        /**
-        * 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 @@
         * @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 @@
                        }
 
                        # 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 @@
                        $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;
diff --git a/includes/filebackend/TempFSFile.php 
b/includes/filebackend/TempFSFile.php
index f572840..fed6812 100644
--- a/includes/filebackend/TempFSFile.php
+++ b/includes/filebackend/TempFSFile.php
@@ -48,15 +48,20 @@
         * 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();
@@ -74,6 +79,40 @@
        }
 
        /**
+        * @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
         *
         * @return bool Success
diff --git a/includes/filerepo/file/File.php b/includes/filerepo/file/File.php
index 425a08c..c48866b 100644
--- a/includes/filerepo/file/File.php
+++ b/includes/filerepo/file/File.php
@@ -1328,7 +1328,7 @@
         */
        protected function makeTransformTmpFile( $thumbPath ) {
                $thumbExt = FileBackend::extensionFromPath( $thumbPath );
-               return TempFSFile::factory( 'transform_', $thumbExt );
+               return TempFSFile::factory( 'transform_', $thumbExt, 
wfTempDir() );
        }
 
        /**
diff --git a/includes/upload/UploadFromChunks.php 
b/includes/upload/UploadFromChunks.php
index 9145a85..08cf434 100644
--- a/includes/upload/UploadFromChunks.php
+++ b/includes/upload/UploadFromChunks.php
@@ -130,7 +130,7 @@
                // 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
diff --git a/includes/upload/UploadFromUrl.php 
b/includes/upload/UploadFromUrl.php
index 6639c34..865f630 100644
--- a/includes/upload/UploadFromUrl.php
+++ b/includes/upload/UploadFromUrl.php
@@ -202,7 +202,7 @@
         * @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();
diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php 
b/tests/phpunit/includes/filebackend/FileBackendTest.php
index 254cfbd..ca562aa 100644
--- a/tests/phpunit/includes/filebackend/FileBackendTest.php
+++ b/tests/phpunit/includes/filebackend/FileBackendTest.php
@@ -268,7 +268,7 @@
        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 @@
                $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 );
diff --git a/tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php 
b/tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php
index ed80c57..92a54fa 100644
--- a/tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php
+++ b/tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php
@@ -59,7 +59,8 @@
                        ->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 );
 
diff --git a/tests/phpunit/mocks/filebackend/MockFSFile.php 
b/tests/phpunit/mocks/filebackend/MockFSFile.php
index bdeed58..6797f59 100644
--- a/tests/phpunit/mocks/filebackend/MockFSFile.php
+++ b/tests/phpunit/mocks/filebackend/MockFSFile.php
@@ -50,15 +50,11 @@
                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(),
                ];
        }

-- 
To view, visit https://gerrit.wikimedia.org/r/311362
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Idd55936b07f9448a6c90577708722b7b52b8fe66
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: Anomie <bjor...@wikimedia.org>
Gerrit-Reviewer: BryanDavis <bda...@wikimedia.org>
Gerrit-Reviewer: Catrope <roan.katt...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to