TempFSFileFactory service
authorAryeh Gregor <ayg@aryeh.name>
Fri, 16 Aug 2019 10:00:15 +0000 (13:00 +0300)
committerAryeh Gregor <ayg@aryeh.name>
Wed, 21 Aug 2019 13:26:05 +0000 (16:26 +0300)
This replaces TempFSFile::factory(), which is now deprecated.

Change-Id: I9e65c3867e26c16687560dccc7d9f3e195a8bdd6

17 files changed:
RELEASE-NOTES-1.34
autoload.php
includes/MediaWikiServices.php
includes/ServiceWiring.php
includes/api/ApiImageRotate.php
includes/filebackend/FileBackendGroup.php
includes/filerepo/file/File.php
includes/libs/filebackend/FSFileBackend.php
includes/libs/filebackend/FileBackend.php
includes/libs/filebackend/MemoryFileBackend.php
includes/libs/filebackend/SwiftFileBackend.php
includes/libs/filebackend/fsfile/TempFSFile.php
includes/libs/filebackend/fsfile/TempFSFileFactory.php [new file with mode: 0644]
includes/upload/UploadFromChunks.php
includes/upload/UploadFromUrl.php
tests/phpunit/includes/libs/filebackend/fsfile/TempFSFileIntegrationTest.php
tests/phpunit/unit/includes/libs/filebackend/fsfile/TempFSFileTest.php [new file with mode: 0644]

index 6e78948..a05f8d1 100644 (file)
@@ -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 ===
 * …
index acdf8dd..20c19e5 100644 (file)
@@ -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',
index bb9c05f..ec82f8e 100644 (file)
@@ -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
index 0b4ce4a..8b7500f 100644 (file)
@@ -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' );
        },
index 668bd0e..ccb26a8 100644 (file)
@@ -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,
index 7ec2357..db7141f 100644 (file)
@@ -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 );
                }
index ee7ee6f..5f6a0cb 100644 (file)
@@ -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 );
        }
 
        /**
index 593e617..e6a49c7 100644 (file)
@@ -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 {
index f65619f..b187fe5 100644 (file)
@@ -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 ) {
index 548c85c..3932f34 100644 (file)
@@ -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'] ) ) {
index a1b2460..d6a0c6c 100644 (file)
@@ -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 ) {
index b993626..378dbe7 100644 (file)
@@ -1,4 +1,7 @@
 <?php
+
+use MediaWiki\FileBackend\FSFile\TempFSFileFactory;
+
 /**
  * Location holder of files stored temporarily
  *
@@ -34,12 +37,17 @@ class TempFSFile extends FSFile {
        /** @var array Map of (path => 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 (file)
index 0000000..1120973
--- /dev/null
@@ -0,0 +1,56 @@
+<?php
+
+namespace MediaWiki\FileBackend\FSFile;
+
+use TempFSFile;
+
+/**
+ * @ingroup FileBackend
+ */
+class TempFSFileFactory {
+       /** @var string|null */
+       private $tmpDirectory;
+
+       /**
+        * @param string|null $tmpDirectory A directory to put the temporary files in, e.g.,
+        *   $wgTmpDirectory. If null, we'll try to find one ourselves.
+        */
+       public function __construct( $tmpDirectory = null ) {
+               $this->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
+       }
+}
index cc527e7..8c6b2f9 100644 (file)
@@ -1,4 +1,7 @@
 <?php
+
+use MediaWiki\MediaWikiServices;
+
 /**
  * Backend for uploading files from chunks.
  *
@@ -157,7 +160,8 @@ 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, wfTempDir() );
+               $tmpFile = MediaWikiServices::getInstance()->getTempFSFileFactory()
+                       ->getTempFSFile( 'chunkedupload_', $ext );
                $tmpPath = false; // fail in concatenate()
                if ( $tmpFile ) {
                        // keep alive with $this
index b071774..b92fcc5 100644 (file)
@@ -1,4 +1,7 @@
 <?php
+
+use MediaWiki\MediaWikiServices;
+
 /**
  * Backend for uploading files from a HTTP resource.
  *
@@ -201,7 +204,8 @@ class UploadFromUrl extends UploadBase {
         * @return string Path to the file
         */
        protected function makeTemporaryFile() {
-               $tmpFile = TempFSFile::factory( 'URL', 'urlupload_', wfTempDir() );
+               $tmpFile = MediaWikiServices::getInstance()->getTempFSFileFactory()
+                       ->newTempFSFile( 'URL', 'urlupload_' );
                $tmpFile->bind( $this );
 
                return $tmpFile->getPath();
index 42805b2..325babc 100644 (file)
@@ -1,6 +1,22 @@
 <?php
 
+use MediaWiki\MediaWikiServices;
+use Wikimedia\TestingAccessWrapper;
+
+/**
+ * Just to test one deprecated method and one line of ServiceWiring code.
+ */
 class TempFSFileIntegrationTest extends MediaWikiIntegrationTestCase {
+       /**
+        * @coversNothing
+        */
+       public function testServiceWiring() {
+               $this->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 (file)
index 0000000..743ac63
--- /dev/null
@@ -0,0 +1,16 @@
+<?php
+
+use MediaWiki\FileBackend\FSFile\TempFSFileFactory;
+
+/**
+ * @coversDefaultClass \MediaWiki\FileBackend\FSFile\TempFSFileFactory
+ * @covers ::__construct
+ * @covers ::newTempFSFile
+ */
+class TempFSFileTest extends MediaWikiUnitTestCase {
+       use TempFSFileTestTrait;
+
+       private function newFile() {
+               return ( new TempFSFileFactory() )->newTempFSFile( 'tmp' );
+       }
+}