Merge "Make UploadBase use TempFSFile to wrap the temporary file"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Thu, 10 Mar 2016 20:04:54 +0000 (20:04 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Thu, 10 Mar 2016 20:04:54 +0000 (20:04 +0000)
includes/filerepo/FileRepo.php
includes/filerepo/LocalRepo.php
includes/filerepo/file/File.php
includes/filerepo/file/LocalFile.php
includes/upload/UploadBase.php
includes/upload/UploadFromChunks.php

index f3c2abf..1565b49 100644 (file)
@@ -1158,7 +1158,7 @@ class FileRepo {
         * Options to $options include:
         *   - headers : name/value map of HTTP headers to use in response to GET/HEAD requests
         *
-        * @param string $srcPath The source file system path, storage path, or URL
+        * @param string|FSFile $src The source file system path, storage path, or URL
         * @param string $dstRel The destination relative path
         * @param string $archiveRel The relative path where the existing file is to
         *   be archived, if there is one. Relative to the public zone root.
@@ -1168,12 +1168,12 @@ class FileRepo {
         * @return FileRepoStatus
         */
        public function publish(
-               $srcPath, $dstRel, $archiveRel, $flags = 0, array $options = []
+               $src, $dstRel, $archiveRel, $flags = 0, array $options = []
        ) {
                $this->assertWritableRepo(); // fail out if read-only
 
                $status = $this->publishBatch(
-                       [ [ $srcPath, $dstRel, $archiveRel, $options ] ], $flags );
+                       [ [ $src, $dstRel, $archiveRel, $options ] ], $flags );
                if ( $status->successCount == 0 ) {
                        $status->ok = false;
                }
@@ -1212,7 +1212,9 @@ class FileRepo {
                $sourceFSFilesToDelete = []; // cleanup for disk source files
                // Validate each triplet and get the store operation...
                foreach ( $ntuples as $ntuple ) {
-                       list( $srcPath, $dstRel, $archiveRel ) = $ntuple;
+                       list( $src, $dstRel, $archiveRel ) = $ntuple;
+                       $srcPath = ( $src instanceof FSFile ) ? $src->getPath() : $src;
+
                        $options = isset( $ntuple[3] ) ? $ntuple[3] : [];
                        // Resolve source to a storage path if virtual
                        $srcPath = $this->resolveToStoragePath( $srcPath );
@@ -1275,7 +1277,7 @@ class FileRepo {
                        } else { // FS source path
                                $operations[] = [
                                        'op' => 'store',
-                                       'src' => $srcPath,
+                                       'src' => $src, // prefer FSFile objects
                                        'dst' => $dstPath,
                                        'overwrite' => true, // replace current
                                        'headers' => $headers
index 5e9a4a9..d24aa24 100644 (file)
@@ -532,7 +532,7 @@ class LocalRepo extends FileRepo {
        }
 
        public function publish(
-               $srcPath,
+               $src,
                $dstRel,
                $archiveRel,
                $flags = 0,
index 6a1bb92..433e395 100644 (file)
@@ -1802,7 +1802,7 @@ abstract class File implements IDBAccessObject {
         * Options to $options include:
         *   - headers : name/value map of HTTP headers to use in response to GET/HEAD requests
         *
-        * @param string $srcPath Local filesystem path to the source image
+        * @param string|FSFile $src Local filesystem path to the source image
         * @param int $flags A bitwise combination of:
         *   File::DELETE_SOURCE    Delete the source file, i.e. move rather than copy
         * @param array $options Optional additional parameters
@@ -1812,7 +1812,7 @@ abstract class File implements IDBAccessObject {
         * STUB
         * Overridden by LocalFile
         */
-       function publish( $srcPath, $flags = 0, array $options = [] ) {
+       function publish( $src, $flags = 0, array $options = [] ) {
                $this->readOnlyError();
        }
 
index c97f38f..0a3ded7 100644 (file)
@@ -1108,7 +1108,7 @@ class LocalFile extends File {
 
        /**
         * Upload a file and record it in the DB
-        * @param string $srcPath Source storage path, virtual URL, or filesystem path
+        * @param string|FSFile $src Source storage path, virtual URL, or filesystem path
         * @param string $comment Upload description
         * @param string $pageText Text to use for the new description page,
         *   if a new description page is created
@@ -1124,7 +1124,7 @@ class LocalFile extends File {
         * @return FileRepoStatus On success, the value member contains the
         *     archive name, or an empty string if it was a new file.
         */
-       function upload( $srcPath, $comment, $pageText, $flags = 0, $props = false,
+       function upload( $src, $comment, $pageText, $flags = 0, $props = false,
                $timestamp = false, $user = null, $tags = []
        ) {
                global $wgContLang;
@@ -1133,6 +1133,7 @@ class LocalFile extends File {
                        return $this->readOnlyFatalStatus();
                }
 
+               $srcPath = ( $src instanceof FSFile ) ? $src->getPath() : $src;
                if ( !$props ) {
                        if ( $this->repo->isVirtualUrl( $srcPath )
                                || FileBackend::isStoragePath( $srcPath )
@@ -1158,7 +1159,7 @@ class LocalFile extends File {
                // non-nicely (dangling multi-byte chars, non-truncated version in cache).
                $comment = $wgContLang->truncate( $comment, 255 );
                $this->lock(); // begin
-               $status = $this->publish( $srcPath, $flags, $options );
+               $status = $this->publish( $src, $flags, $options );
 
                if ( $status->successCount >= 2 ) {
                        // There will be a copy+(one of move,copy,store).
@@ -1523,15 +1524,15 @@ class LocalFile extends File {
         * The archive name should be passed through to recordUpload for database
         * registration.
         *
-        * @param string $srcPath Local filesystem path or virtual URL to the source image
+        * @param string|FSFile $src Local filesystem path or virtual URL to the source image
         * @param int $flags A bitwise combination of:
         *     File::DELETE_SOURCE    Delete the source file, i.e. move rather than copy
         * @param array $options Optional additional parameters
         * @return FileRepoStatus On success, the value member contains the
         *     archive name, or an empty string if it was a new file.
         */
-       function publish( $srcPath, $flags = 0, array $options = [] ) {
-               return $this->publishTo( $srcPath, $this->getRel(), $flags, $options );
+       function publish( $src, $flags = 0, array $options = [] ) {
+               return $this->publishTo( $src, $this->getRel(), $flags, $options );
        }
 
        /**
@@ -1541,7 +1542,7 @@ class LocalFile extends File {
         * The archive name should be passed through to recordUpload for database
         * registration.
         *
-        * @param string $srcPath Local filesystem path or virtual URL to the source image
+        * @param string|FSFile $src Local filesystem path or virtual URL to the source image
         * @param string $dstRel Target relative path
         * @param int $flags A bitwise combination of:
         *     File::DELETE_SOURCE    Delete the source file, i.e. move rather than copy
@@ -1549,7 +1550,9 @@ class LocalFile extends File {
         * @return FileRepoStatus On success, the value member contains the
         *     archive name, or an empty string if it was a new file.
         */
-       function publishTo( $srcPath, $dstRel, $flags = 0, array $options = [] ) {
+       function publishTo( $src, $dstRel, $flags = 0, array $options = [] ) {
+               $srcPath = ( $src instanceof FSFile ) ? $src->getPath() : $src;
+
                $repo = $this->getRepo();
                if ( $repo->getReadOnlyReason() !== false ) {
                        return $this->readOnlyFatalStatus();
@@ -1563,9 +1566,9 @@ class LocalFile extends File {
                if ( $repo->hasSha1Storage() ) {
                        $sha1 = $repo->isVirtualUrl( $srcPath )
                                ? $repo->getFileSha1( $srcPath )
-                               : File::sha1Base36( $srcPath );
+                               : FSFile::getSha1Base36FromPath( $srcPath );
                        $dst = $repo->getBackend()->getPathForSHA1( $sha1 );
-                       $status = $repo->quickImport( $srcPath, $dst );
+                       $status = $repo->quickImport( $src, $dst );
                        if ( $flags & File::DELETE_SOURCE ) {
                                unlink( $srcPath );
                        }
index a874038..fb25249 100644 (file)
  * @author Michael Dale
  */
 abstract class UploadBase {
+       /** @var string Local file system path to the file to upload (or a local copy) */
        protected $mTempPath;
+       /** @var TempFSFile|null Wrapper to handle deleting the temp file */
+       protected $tempFileObj;
+
        protected $mDesiredDestName, $mDestName, $mRemoveTempFile, $mSourceType;
        protected $mTitle = false, $mTitleError = 0;
        protected $mFilteredName, $mFinalExtension;
@@ -219,8 +223,8 @@ abstract class UploadBase {
                if ( FileBackend::isStoragePath( $tempPath ) ) {
                        throw new MWException( __METHOD__ . " given storage path `$tempPath`." );
                }
-               $this->mTempPath = $tempPath;
-               $this->mFileSize = $fileSize;
+
+               $this->setTempFile( $tempPath, $fileSize );
                $this->mRemoveTempFile = $removeTempFile;
        }
 
@@ -231,6 +235,21 @@ abstract class UploadBase {
         */
        abstract public function initializeFromRequest( &$request );
 
+       /**
+        * @param string $tempPath File system path to temporary file containing the upload
+        * @param integer $fileSize
+        */
+       protected function setTempFile( $tempPath, $fileSize = null ) {
+               $this->mTempPath = $tempPath;
+               if ( strlen( $this->mTempPath ) && file_exists( $this->mTempPath ) ) {
+                       $this->tempFileObj = new TempFSFile( $this->mTempPath );
+                       $this->mFileSize = $fileSize ?: filesize( $this->mTempPath );
+               } else {
+                       $this->tempFileObj = null;
+                       $this->mFileSize = null;
+               }
+       }
+
        /**
         * Fetch the file. Usually a no-op
         * @return Status
@@ -952,9 +971,10 @@ abstract class UploadBase {
         * on exit to clean up.
         */
        public function cleanupTempFile() {
-               if ( $this->mRemoveTempFile && $this->mTempPath && file_exists( $this->mTempPath ) ) {
-                       wfDebug( __METHOD__ . ": Removing temporary file {$this->mTempPath}\n" );
-                       unlink( $this->mTempPath );
+               if ( $this->mRemoveTempFile && $this->tempFileObj ) {
+                       // Delete when all relevant TempFSFile handles go out of scope
+                       wfDebug( __METHOD__ . ": Marked temporary file '{$this->mTempPath}' for removal\n" );
+                       $this->tempFileObj->autocollect();
                }
        }
 
index 8ee0845..ebb4ebb 100644 (file)
@@ -148,10 +148,9 @@ class UploadFromChunks extends UploadFromFile {
 
                wfDebugLog( 'fileconcatenate', "Combined $i chunks in $tAmount seconds." );
 
-               // File system path
-               $this->mTempPath = $tmpPath;
-               // Since this was set for the last chunk previously
-               $this->mFileSize = filesize( $this->mTempPath );
+               // File system path of the actual full temp file
+               $this->setTempFile( $tmpPath );
+
                $ret = $this->verifyUpload();
                if ( $ret['status'] !== UploadBase::OK ) {
                        wfDebugLog( 'fileconcatenate', "Verification failed for chunked upload" );