From f7e3ac3f95ce2b5a9f73326e854d1c301b513078 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sun, 18 Sep 2016 17:22:36 -0700 Subject: [PATCH] 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 --- includes/GlobalFunctions.php | 30 +------------ includes/api/ApiImageRotate.php | 2 +- includes/filebackend/FSFile.php | 19 ++------ includes/filebackend/TempFSFile.php | 45 +++++++++++++++++-- includes/filerepo/file/File.php | 2 +- includes/upload/UploadFromChunks.php | 2 +- includes/upload/UploadFromUrl.php | 2 +- .../includes/filebackend/FileBackendTest.php | 8 ++-- .../filerepo/MigrateFileRepoLayoutTest.php | 3 +- .../phpunit/mocks/filebackend/MockFSFile.php | 6 +-- 10 files changed, 57 insertions(+), 62 deletions(-) diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index 90bba536d2..e5f518ffd2 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -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(); } /** diff --git a/includes/api/ApiImageRotate.php b/includes/api/ApiImageRotate.php index 2b9935341b..966bcbf6b6 100644 --- a/includes/api/ApiImageRotate.php +++ b/includes/api/ApiImageRotate.php @@ -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, diff --git a/includes/filebackend/FSFile.php b/includes/filebackend/FSFile.php index 8aa11b6565..221fe9fce2 100644 --- a/includes/filebackend/FSFile.php +++ b/includes/filebackend/FSFile.php @@ -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; diff --git a/includes/filebackend/TempFSFile.php b/includes/filebackend/TempFSFile.php index f57284080d..fed6812f5b 100644 --- a/includes/filebackend/TempFSFile.php +++ b/includes/filebackend/TempFSFile.php @@ -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 * diff --git a/includes/filerepo/file/File.php b/includes/filerepo/file/File.php index 425a08cb04..c48866b5a7 100644 --- a/includes/filerepo/file/File.php +++ b/includes/filerepo/file/File.php @@ -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() ); } /** diff --git a/includes/upload/UploadFromChunks.php b/includes/upload/UploadFromChunks.php index 9145a854f3..08cf434d47 100644 --- a/includes/upload/UploadFromChunks.php +++ b/includes/upload/UploadFromChunks.php @@ -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 diff --git a/includes/upload/UploadFromUrl.php b/includes/upload/UploadFromUrl.php index 6639c340ae..865f6300be 100644 --- a/includes/upload/UploadFromUrl.php +++ b/includes/upload/UploadFromUrl.php @@ -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(); diff --git a/tests/phpunit/includes/filebackend/FileBackendTest.php b/tests/phpunit/includes/filebackend/FileBackendTest.php index 254cfbd677..ca562aa14f 100644 --- a/tests/phpunit/includes/filebackend/FileBackendTest.php +++ b/tests/phpunit/includes/filebackend/FileBackendTest.php @@ -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 ); diff --git a/tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php b/tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php index ed80c573a2..92a54faca1 100644 --- a/tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php +++ b/tests/phpunit/includes/filerepo/MigrateFileRepoLayoutTest.php @@ -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 ); diff --git a/tests/phpunit/mocks/filebackend/MockFSFile.php b/tests/phpunit/mocks/filebackend/MockFSFile.php index bdeed58a81..6797f59554 100644 --- a/tests/phpunit/mocks/filebackend/MockFSFile.php +++ b/tests/phpunit/mocks/filebackend/MockFSFile.php @@ -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(), ]; } -- 2.20.1