From: Aryeh Gregor Date: Fri, 16 Aug 2019 10:00:15 +0000 (+0300) Subject: TempFSFileFactory service X-Git-Tag: 1.34.0-rc.0~514^2~1 X-Git-Url: http://git.cyclocoop.org/data/%24oldEdit?a=commitdiff_plain;h=a83b33582ae87efff13e68b67585ee0e1aeed036;p=lhc%2Fweb%2Fwiklou.git TempFSFileFactory service This replaces TempFSFile::factory(), which is now deprecated. Change-Id: I9e65c3867e26c16687560dccc7d9f3e195a8bdd6 --- diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 6e789487be..a05f8d18e1 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -453,6 +453,7 @@ because of Phabricator reports. * MessageCache::singleton() is deprecated. Use MediaWikiServices::getMessageCache(). * Constructing MovePage directly is deprecated. Use MovePageFactory. +* TempFSFile::factory() has been deprecated. Use TempFSFileFactory instead. === Other changes in 1.34 === * … diff --git a/autoload.php b/autoload.php index acdf8dd3d2..20c19e50a9 100644 --- a/autoload.php +++ b/autoload.php @@ -880,6 +880,7 @@ $wgAutoloadLocalClasses = [ 'MediaWiki\\DB\\PatchFileLocation' => __DIR__ . '/includes/db/PatchFileLocation.php', 'MediaWiki\\Diff\\ComplexityException' => __DIR__ . '/includes/diff/ComplexityException.php', 'MediaWiki\\Diff\\WordAccumulator' => __DIR__ . '/includes/diff/WordAccumulator.php', + 'MediaWiki\\FileBackend\\FSFile\\TempFSFileFactory' => __DIR__ . '/includes/libs/filebackend/fsfile/TempFSFileFactory.php', 'MediaWiki\\HeaderCallback' => __DIR__ . '/includes/HeaderCallback.php', 'MediaWiki\\Http\\HttpRequestFactory' => __DIR__ . '/includes/http/HttpRequestFactory.php', 'MediaWiki\\Installer\\InstallException' => __DIR__ . '/includes/installer/InstallException.php', diff --git a/includes/MediaWikiServices.php b/includes/MediaWikiServices.php index bb9c05f424..ec82f8ed28 100644 --- a/includes/MediaWikiServices.php +++ b/includes/MediaWikiServices.php @@ -16,6 +16,7 @@ use IBufferingStatsdDataFactory; use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use MediaWiki\Block\BlockManager; use MediaWiki\Block\BlockRestrictionStore; +use MediaWiki\FileBackend\FSFile\TempFSFileFactory; use MediaWiki\Http\HttpRequestFactory; use MediaWiki\Page\MovePageFactory; use MediaWiki\Permissions\PermissionManager; @@ -964,6 +965,14 @@ class MediaWikiServices extends ServiceContainer { return $this->getService( 'StatsdDataFactory' ); } + /** + * @since 1.34 + * @return TempFSFileFactory + */ + public function getTempFSFileFactory() : TempFSFileFactory { + return $this->getService( 'TempFSFileFactory' ); + } + /** * @since 1.28 * @return TitleFormatter diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 0b4ce4aa3a..8b7500f1a0 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -43,6 +43,7 @@ use MediaWiki\Block\BlockManager; use MediaWiki\Block\BlockRestrictionStore; use MediaWiki\Config\ConfigRepository; use MediaWiki\Config\ServiceOptions; +use MediaWiki\FileBackend\FSFile\TempFSFileFactory; use MediaWiki\Http\HttpRequestFactory; use MediaWiki\Interwiki\ClassicInterwikiLookup; use MediaWiki\Interwiki\InterwikiLookup; @@ -715,6 +716,10 @@ return [ ); }, + 'TempFSFileFactory' => function ( MediaWikiServices $services ) : TempFSFileFactory { + return new TempFSFileFactory( $services->getMainConfig()->get( 'TmpDirectory' ) ); + }, + 'TitleFormatter' => function ( MediaWikiServices $services ) : TitleFormatter { return $services->getService( '_MediaWikiTitleCodec' ); }, diff --git a/includes/api/ApiImageRotate.php b/includes/api/ApiImageRotate.php index 668bd0e427..ccb26a8392 100644 --- a/includes/api/ApiImageRotate.php +++ b/includes/api/ApiImageRotate.php @@ -101,7 +101,8 @@ class ApiImageRotate extends ApiBase { continue; } $ext = strtolower( pathinfo( "$srcPath", PATHINFO_EXTENSION ) ); - $tmpFile = TempFSFile::factory( 'rotate_', $ext, wfTempDir() ); + $tmpFile = MediaWikiServices::getInstance()->getTempFSFileFactory() + ->newTempFSFile( 'rotate_', $ext ); $dstPath = $tmpFile->getPath(); $err = $handler->rotate( $file, [ 'srcPath' => $srcPath, diff --git a/includes/filebackend/FileBackendGroup.php b/includes/filebackend/FileBackendGroup.php index 7ec2357e5b..db7141f76b 100644 --- a/includes/filebackend/FileBackendGroup.php +++ b/includes/filebackend/FileBackendGroup.php @@ -186,7 +186,7 @@ class FileBackendGroup { 'mimeCallback' => [ $this, 'guessMimeInternal' ], 'obResetFunc' => 'wfResetOutputBuffers', 'streamMimeFunc' => [ StreamFile::class, 'contentTypeFromPath' ], - 'tmpDirectory' => wfTempDir(), + 'tmpFileFactory' => MediaWikiServices::getInstance()->getTempFSFileFactory(), 'statusWrapper' => [ Status::class, 'wrap' ], 'wanCache' => $services->getMainWANObjectCache(), 'srvCache' => ObjectCache::getLocalServerInstance( 'hash' ), @@ -241,7 +241,8 @@ class FileBackendGroup { if ( !$type && $fsPath ) { $type = $magic->guessMimeType( $fsPath, false ); } elseif ( !$type && strlen( $content ) ) { - $tmpFile = TempFSFile::factory( 'mime_', '', wfTempDir() ); + $tmpFile = MediaWikiServices::getInstance()->getTempFSFileFactory() + ->newTempFSFile( 'mime_', '' ); file_put_contents( $tmpFile->getPath(), $content ); $type = $magic->guessMimeType( $tmpFile->getPath(), false ); } diff --git a/includes/filerepo/file/File.php b/includes/filerepo/file/File.php index ee7ee6f90d..5f6a0cbcd7 100644 --- a/includes/filerepo/file/File.php +++ b/includes/filerepo/file/File.php @@ -1352,7 +1352,8 @@ abstract class File implements IDBAccessObject { */ protected function makeTransformTmpFile( $thumbPath ) { $thumbExt = FileBackend::extensionFromPath( $thumbPath ); - return TempFSFile::factory( 'transform_', $thumbExt, wfTempDir() ); + return MediaWikiServices::getInstance()->getTempFSFileFactory() + ->newTempFSFile( 'transform_', $thumbExt ); } /** diff --git a/includes/libs/filebackend/FSFileBackend.php b/includes/libs/filebackend/FSFileBackend.php index 593e617fe6..e6a49c7dd4 100644 --- a/includes/libs/filebackend/FSFileBackend.php +++ b/includes/libs/filebackend/FSFileBackend.php @@ -228,7 +228,7 @@ class FSFileBackend extends FileBackendStore { } if ( !empty( $params['async'] ) ) { // deferred - $tempFile = TempFSFile::factory( 'create_', 'tmp', $this->tmpDirectory ); + $tempFile = $this->tmpFileFactory->newTempFSFile( 'create_', 'tmp' ); if ( !$tempFile ) { $status->fatal( 'backend-fail-create', $params['dst'] ); @@ -688,7 +688,7 @@ class FSFileBackend extends FileBackendStore { } else { // Create a new temporary file with the same extension... $ext = FileBackend::extensionFromPath( $src ); - $tmpFile = TempFSFile::factory( 'localcopy_', $ext, $this->tmpDirectory ); + $tmpFile = $this->tmpFileFactory->newTempFSFile( 'localcopy_', $ext ); if ( !$tmpFile ) { $tmpFiles[$src] = null; } else { diff --git a/includes/libs/filebackend/FileBackend.php b/includes/libs/filebackend/FileBackend.php index f65619fd8c..b187fe563c 100644 --- a/includes/libs/filebackend/FileBackend.php +++ b/includes/libs/filebackend/FileBackend.php @@ -27,6 +27,7 @@ * @file * @ingroup FileBackend */ +use MediaWiki\FileBackend\FSFile\TempFSFileFactory; use Psr\Log\LoggerAwareInterface; use Psr\Log\LoggerInterface; use Wikimedia\ScopedCallback; @@ -106,8 +107,8 @@ abstract class FileBackend implements LoggerAwareInterface { /** @var int How many operations can be done in parallel */ protected $concurrency; - /** @var string Temporary file directory */ - protected $tmpDirectory; + /** @var TempFSFileFactory */ + protected $tmpFileFactory; /** @var LockManager */ protected $lockManager; @@ -152,8 +153,10 @@ abstract class FileBackend implements LoggerAwareInterface { * - parallelize : When to do file operations in parallel (when possible). * Allowed values are "implicit", "explicit" and "off". * - concurrency : How many file operations can be done in parallel. - * - tmpDirectory : Directory to use for temporary files. If this is not set or null, - * then the backend will try to discover a usable temporary directory. + * - tmpDirectory : Directory to use for temporary files. + * - tmpFileFactory : Optional TempFSFileFactory object. Only has an effect if tmpDirectory is + * not set. If both are unset or null, then the backend will try to discover a usable + * temporary directory. * - obResetFunc : alternative callback to clear the output buffer * - streamMimeFunc : alternative method to determine the content type from the path * - logger : Optional PSR logger object. @@ -193,7 +196,12 @@ abstract class FileBackend implements LoggerAwareInterface { } $this->logger = $config['logger'] ?? new NullLogger(); $this->statusWrapper = $config['statusWrapper'] ?? null; - $this->tmpDirectory = $config['tmpDirectory'] ?? null; + // tmpDirectory gets precedence for backward compatibility + if ( isset( $config['tmpDirectory'] ) ) { + $this->tmpFileFactory = new TempFSFileFactory( $config['tmpDirectory'] ); + } else { + $this->tmpFileFactory = $config['tmpFileFactory'] ?? new TempFSFileFactory(); + } } public function setLogger( LoggerInterface $logger ) { diff --git a/includes/libs/filebackend/MemoryFileBackend.php b/includes/libs/filebackend/MemoryFileBackend.php index 548c85c85d..3932f34505 100644 --- a/includes/libs/filebackend/MemoryFileBackend.php +++ b/includes/libs/filebackend/MemoryFileBackend.php @@ -168,7 +168,7 @@ class MemoryFileBackend extends FileBackendStore { } else { // Create a new temporary file with the same extension... $ext = FileBackend::extensionFromPath( $src ); - $fsFile = TempFSFile::factory( 'localcopy_', $ext, $this->tmpDirectory ); + $fsFile = $this->tmpFileFactory->newTempFSFile( 'localcopy_', $ext ); if ( $fsFile ) { $bytes = file_put_contents( $fsFile->getPath(), $this->files[$src]['data'] ); if ( $bytes !== strlen( $this->files[$src]['data'] ) ) { diff --git a/includes/libs/filebackend/SwiftFileBackend.php b/includes/libs/filebackend/SwiftFileBackend.php index a1b2460df6..d6a0c6c0a3 100644 --- a/includes/libs/filebackend/SwiftFileBackend.php +++ b/includes/libs/filebackend/SwiftFileBackend.php @@ -1150,7 +1150,7 @@ class SwiftFileBackend extends FileBackendStore { // Get source file extension $ext = FileBackend::extensionFromPath( $path ); // Create a new temporary file... - $tmpFile = TempFSFile::factory( 'localcopy_', $ext, $this->tmpDirectory ); + $tmpFile = $this->tmpFileFactory->newTempFSFile( 'localcopy_', $ext ); if ( $tmpFile ) { $handle = fopen( $tmpFile->getPath(), 'wb' ); if ( $handle ) { diff --git a/includes/libs/filebackend/fsfile/TempFSFile.php b/includes/libs/filebackend/fsfile/TempFSFile.php index b993626fda..378dbe7268 100644 --- a/includes/libs/filebackend/fsfile/TempFSFile.php +++ b/includes/libs/filebackend/fsfile/TempFSFile.php @@ -1,4 +1,7 @@ 1) for paths to delete on shutdown */ protected static $pathsCollect = null; + /** + * Do not call directly. Use TempFSFileFactory. + */ public function __construct( $path ) { parent::__construct( $path ); if ( self::$pathsCollect === null ) { + // @codeCoverageIgnoreStart self::$pathsCollect = []; register_shutdown_function( [ __CLASS__, 'purgeAllOnShutdown' ] ); + // @codeCoverageIgnoreEnd } } @@ -47,40 +55,23 @@ class TempFSFile extends FSFile { * Make a new temporary file on the file system. * Temporary files may be purged when the file object falls out of scope. * + * @deprecated since 1.34, use TempFSFileFactory directly + * * @param string $prefix * @param string $extension Optional file extension * @param string|null $tmpDirectory Optional parent directory * @return TempFSFile|null */ public static function factory( $prefix, $extension = '', $tmpDirectory = null ) { - $ext = ( $extension != '' ) ? ".{$extension}" : ''; - - $attempts = 5; - while ( $attempts-- ) { - $hex = sprintf( '%06x%06x', mt_rand( 0, 0xffffff ), mt_rand( 0, 0xffffff ) ); - if ( !is_string( $tmpDirectory ) ) { - $tmpDirectory = self::getUsableTempDirectory(); - } - $path = $tmpDirectory . '/' . $prefix . $hex . $ext; - Wikimedia\suppressWarnings(); - $newFileHandle = fopen( $path, 'x' ); - Wikimedia\restoreWarnings(); - if ( $newFileHandle ) { - fclose( $newFileHandle ); - $tmpFile = new self( $path ); - $tmpFile->autocollect(); - // Safely instantiated, end loop. - return $tmpFile; - } - } - - // Give up - return null; + return ( new TempFSFileFactory( $tmpDirectory ) )->newTempFSFile( $prefix, $extension ); } /** + * @todo Is there any useful way to test this? Would it be useful to make this non-static on + * TempFSFileFactory? + * * @return string Filesystem path to a temporary directory - * @throws RuntimeException + * @throws RuntimeException if no writable temporary directory can be found */ public static function getUsableTempDirectory() { $tmpDir = array_map( 'getenv', [ 'TMPDIR', 'TMP', 'TEMP' ] ); @@ -176,6 +167,8 @@ class TempFSFile extends FSFile { * Try to make sure that all files are purged on error * * This method should only be called internally + * + * @codeCoverageIgnore */ public static function purgeAllOnShutdown() { foreach ( self::$pathsCollect as $path => $unused ) { diff --git a/includes/libs/filebackend/fsfile/TempFSFileFactory.php b/includes/libs/filebackend/fsfile/TempFSFileFactory.php new file mode 100644 index 0000000000..1120973803 --- /dev/null +++ b/includes/libs/filebackend/fsfile/TempFSFileFactory.php @@ -0,0 +1,56 @@ +tmpDirectory = $tmpDirectory; + } + + /** + * Make a new temporary file on the file system. + * Temporary files may be purged when the file object falls out of scope. + * + * @param string $prefix + * @param string $extension Optional file extension + * @return TempFSFile|null + */ + public function newTempFSFile( $prefix, $extension = '' ) { + $ext = ( $extension != '' ) ? ".{$extension}" : ''; + $tmpDirectory = $this->tmpDirectory; + if ( !is_string( $tmpDirectory ) ) { + $tmpDirectory = TempFSFile::getUsableTempDirectory(); + } + + $attempts = 5; + while ( $attempts-- ) { + $hex = sprintf( '%06x%06x', mt_rand( 0, 0xffffff ), mt_rand( 0, 0xffffff ) ); + $path = "$tmpDirectory/$prefix$hex$ext"; + \Wikimedia\suppressWarnings(); + $newFileHandle = fopen( $path, 'x' ); + \Wikimedia\restoreWarnings(); + if ( $newFileHandle ) { + fclose( $newFileHandle ); + $tmpFile = new TempFSFile( $path ); + $tmpFile->autocollect(); + // Safely instantiated, end loop. + return $tmpFile; + } + } + + // Give up + return null; // @codeCoverageIgnore + } +} diff --git a/includes/upload/UploadFromChunks.php b/includes/upload/UploadFromChunks.php index cc527e7e4b..8c6b2f9c05 100644 --- a/includes/upload/UploadFromChunks.php +++ b/includes/upload/UploadFromChunks.php @@ -1,4 +1,7 @@ mVirtualTempPath ); // Get a 0-byte temp file to perform the concatenation at - $tmpFile = TempFSFile::factory( 'chunkedupload_', $ext, wfTempDir() ); + $tmpFile = MediaWikiServices::getInstance()->getTempFSFileFactory() + ->getTempFSFile( 'chunkedupload_', $ext ); $tmpPath = false; // fail in concatenate() if ( $tmpFile ) { // keep alive with $this diff --git a/includes/upload/UploadFromUrl.php b/includes/upload/UploadFromUrl.php index b0717749fe..b92fcc5aa9 100644 --- a/includes/upload/UploadFromUrl.php +++ b/includes/upload/UploadFromUrl.php @@ -1,4 +1,7 @@ getTempFSFileFactory() + ->newTempFSFile( 'URL', 'urlupload_' ); $tmpFile->bind( $this ); return $tmpFile->getPath(); diff --git a/tests/phpunit/includes/libs/filebackend/fsfile/TempFSFileIntegrationTest.php b/tests/phpunit/includes/libs/filebackend/fsfile/TempFSFileIntegrationTest.php index 42805b2db2..325babca4d 100644 --- a/tests/phpunit/includes/libs/filebackend/fsfile/TempFSFileIntegrationTest.php +++ b/tests/phpunit/includes/libs/filebackend/fsfile/TempFSFileIntegrationTest.php @@ -1,6 +1,22 @@ setMwGlobals( 'wgTmpDirectory', '/hopefully invalid' ); + $factory = MediaWikiServices::getInstance()->getTempFSFileFactory(); + $this->assertSame( '/hopefully invalid', + ( TestingAccessWrapper::newFromObject( $factory ) )->tmpDirectory ); + } + use TempFSFileTestTrait; private function newFile() { diff --git a/tests/phpunit/unit/includes/libs/filebackend/fsfile/TempFSFileTest.php b/tests/phpunit/unit/includes/libs/filebackend/fsfile/TempFSFileTest.php new file mode 100644 index 0000000000..743ac63f3e --- /dev/null +++ b/tests/phpunit/unit/includes/libs/filebackend/fsfile/TempFSFileTest.php @@ -0,0 +1,16 @@ +newTempFSFile( 'tmp' ); + } +}