Aaron Schulz has uploaded a new change for review.
https://gerrit.wikimedia.org/r/311362
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 proping 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(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/62/311362/1
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: newchange
Gerrit-Change-Id: Idd55936b07f9448a6c90577708722b7b52b8fe66
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <asch...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
