From: Aaron Schulz <aaron@users.mediawiki.org>
Date: Fri, 23 Dec 2011 18:59:39 +0000 (+0000)
Subject: In FileBackendBase/FileBackend:
X-Git-Tag: 1.31.0-rc.0~25787
X-Git-Url: https://git.cyclocoop.org/%7B%24www_url%7Dadmin/compta/banques/?a=commitdiff_plain;h=875f8a28e938ced147ece1a663cd761c876be0d3;p=lhc%2Fweb%2Fwiklou.git

In FileBackendBase/FileBackend:
* Changed concatenate to store to a specified temp FS file rather than a final storage destination. This makes it better fit the use case (chunked upload), so we can avoid extra copying around. Subclasses no longer have to implement this function now as well.
* Added extensionFromPath() helper function.
* Moved createInternal() up a bit and fixed @see comments pointing to the wrong functions.
In FSFileBackend:
* Use parent implementation of doConcatenateInternal().
In FileRepo/File:
* Added FileRepo::ALLOW_STALE and made thumbnail transforms use it.
---

diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php
index 8983c11dd4..46ffb32414 100644
--- a/includes/filerepo/FileRepo.php
+++ b/includes/filerepo/FileRepo.php
@@ -18,6 +18,7 @@ class FileRepo {
 	const OVERWRITE = 2;
 	const OVERWRITE_SAME = 4;
 	const SKIP_LOCKING = 8;
+	const ALLOW_STALE = 16;
 
 	/** @var FileBackendBase */
 	protected $backend;
@@ -621,6 +622,7 @@ class FileRepo {
 	 *     self::OVERWRITE_SAME    Overwrite the file if the destination exists and has the
 	 *                             same contents as the source
 	 *     self::SKIP_LOCKING      Skip any file locking when doing the store
+	 *     self::ALLOW_STALE       Don't require latest data for existence checks
 	 * @return FileRepoStatus
 	 */
 	public function store( $srcPath, $dstZone, $dstRel, $flags = 0 ) {
@@ -702,6 +704,9 @@ class FileRepo {
 		if ( $flags & self::SKIP_LOCKING ) {
 			$opts['nonLocking'] = true;
 		}
+		if ( $flags & self::ALLOW_STALE ) {
+			$opts['allowStale'] = true;
+		}
 		$status->merge( $backend->doOperations( $operations, $opts ) );
 		// Cleanup for disk source files...
 		foreach ( $sourceFSFilesToDelete as $file ) {
@@ -784,7 +789,7 @@ class FileRepo {
 	 * Concatenate a list of files into a target file location. 
 	 * 
 	 * @param $srcPaths Array Ordered list of source virtual URLs/storage paths
-	 * @param $dstPath String Target virtual URL/storage path
+	 * @param $dstPath String Target file system path
 	 * @param $flags Integer: bitwise combination of the following flags:
 	 *     self::DELETE_SOURCE     Delete the source files
 	 * @return FileRepoStatus
@@ -806,8 +811,7 @@ class FileRepo {
 		}
 
 		// Concatenate the chunks into one file
-		$op = array( 'op' => 'concatenate',
-			'srcs' => $sources, 'dst' => $dest, 'overwriteDest' => true );
+		$op = array( 'op' => 'concatenate', 'srcs' => $sources, 'dst' => $dest );
 		$status->merge( $this->backend->doOperation( $op ) );
 		if ( !$status->isOK() ) {
 			return $status;
diff --git a/includes/filerepo/backend/FSFileBackend.php b/includes/filerepo/backend/FSFileBackend.php
index 83298fed47..eccc1cf5f3 100644
--- a/includes/filerepo/backend/FSFileBackend.php
+++ b/includes/filerepo/backend/FSFileBackend.php
@@ -224,107 +224,6 @@ class FSFileBackend extends FileBackend {
 		return $status;
 	}
 
-	/**
-	 * @see FileBackend::doConcatenateInternal()
-	 */
-	protected function doConcatenateInternal( array $params ) {
-		$status = Status::newGood();
-
-		list( $c, $dest ) = $this->resolveStoragePath( $params['dst'] );
-		if ( $dest === null ) {
-			$status->fatal( 'backend-fail-invalidpath', $params['dst'] );
-			return $status;
-		}
-
-		// Check if the destination file exists and we can't handle that
-		$destExists = file_exists( $dest );
-		if ( $destExists && empty( $params['overwriteDest'] ) ) {
-			$status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
-			return $status;
-		}
-
-		// Create a new temporary file...
-		wfSuppressWarnings();
-		$tmpPath = tempnam( wfTempDir(), 'concatenate' );
-		wfRestoreWarnings();
-		if ( $tmpPath === false ) {
-			$status->fatal( 'backend-fail-createtemp' );
-			return $status;
-		}
-
-		// Build up that file using the source chunks (in order)...
-		wfSuppressWarnings();
-		$tmpHandle = fopen( $tmpPath, 'a' );
-		wfRestoreWarnings();
-		if ( $tmpHandle === false ) {
-			$status->fatal( 'backend-fail-opentemp', $tmpPath );
-			return $status;
-		}
-		foreach ( $params['srcs'] as $virtualSource ) {
-			list( $c, $source ) = $this->resolveStoragePath( $virtualSource );
-			if ( $source === null ) {
-				fclose( $tmpHandle );
-				$status->fatal( 'backend-fail-invalidpath', $virtualSource );
-				return $status;
-			}
-			// Load chunk into memory (it should be a small file)
-			$sourceHandle = fopen( $source, 'r' );
-			if ( $sourceHandle === false ) {
-				fclose( $tmpHandle );
-				$status->fatal( 'backend-fail-read', $virtualSource );
-				return $status;
-			}
-			// Append chunk to file (pass chunk size to avoid magic quotes)
-			if ( !stream_copy_to_stream( $sourceHandle, $tmpHandle ) ) {
-				fclose( $sourceHandle );
-				fclose( $tmpHandle );
-				$status->fatal( 'backend-fail-writetemp', $tmpPath );
-				return $status;
-			}
-			fclose( $sourceHandle );
-		}
-		wfSuppressWarnings();
-		if ( !fclose( $tmpHandle ) ) {
-			$status->fatal( 'backend-fail-closetemp', $tmpPath );
-			return $status;
-		}
-		wfRestoreWarnings();
-
-		// Handle overwrite behavior of file destination if applicable.
-		// Note that we already checked if no overwrite params were set above.
-		if ( $destExists ) {
-			// Windows does not support moving over existing files
-			if ( wfIsWindows() ) {
-				wfSuppressWarnings();
-				$ok = unlink( $dest );
-				wfRestoreWarnings();
-				if ( !$ok ) {
-					$status->fatal( 'backend-fail-delete', $params['dst'] );
-					return $status;
-				}
-			}
-		} else {
-			// Make sure destination directory exists
-			if ( !wfMkdirParents( dirname( $dest ) ) ) {
-				$status->fatal( 'directorycreateerror', $params['dst'] );
-				return $status;
-			}
-		}
-
-		// Rename the temporary file to the destination path
-		wfSuppressWarnings();
-		$ok = rename( $tmpPath, $dest );
-		wfRestoreWarnings();
-		if ( !$ok ) {
-			$status->fatal( 'backend-fail-move', $tmpPath, $params['dst'] );
-			return $status;
-		}
-
-		$this->chmod( $dest );
-
-		return $status;
-	}
-
 	/**
 	 * @see FileBackend::doCreateInternal()
 	 */
diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php
index f6b2a697b6..4bb7332321 100644
--- a/includes/filerepo/backend/FileBackend.php
+++ b/includes/filerepo/backend/FileBackend.php
@@ -112,8 +112,7 @@ abstract class FileBackendBase {
 	 *     array(
 	 *         'op'                  => 'concatenate',
 	 *         'srcs'                => <ordered array of storage paths>,
-	 *         'dst'                 => <storage path>,
-	 *         'overwriteDest'       => <boolean>
+	 *         'dst'                 => <file system path to 0-byte temp file>
 	 *     )
 	 * g) Do nothing (no-op)
 	 *     array(
@@ -476,6 +475,28 @@ abstract class FileBackend extends FileBackendBase {
 	protected $cache = array(); // (storage path => key => value)
 	protected $maxCacheSize = 50; // integer; max paths with entries
 
+	/**
+	 * Create a file in the backend with the given contents.
+	 * Do not call this function from places outside FileBackend and FileOp.
+	 * $params include:
+	 *     content       : the raw file contents
+	 *     dst           : destination storage path
+	 *     overwriteDest : overwrite any file that exists at the destination
+	 * 
+	 * @param $params Array
+	 * @return Status
+	 */
+	final public function createInternal( array $params ) {
+		$status = $this->doCreateInternal( $params );
+		$this->clearCache( array( $params['dst'] ) );
+		return $status;
+	}
+
+	/**
+	 * @see FileBackend::createInternal()
+	 */
+	abstract protected function doCreateInternal( array $params );
+
 	/**
 	 * Store a file into the backend from a file on disk.
 	 * Do not call this function from places outside FileBackend and FileOp.
@@ -537,7 +558,7 @@ abstract class FileBackend extends FileBackendBase {
 	}
 
 	/**
-	 * @see FileBackend::delete()
+	 * @see FileBackend::deleteInternal()
 	 */
 	abstract protected function doDeleteInternal( array $params );
 
@@ -559,7 +580,7 @@ abstract class FileBackend extends FileBackendBase {
 	}
 
 	/**
-	 * @see FileBackend::move()
+	 * @see FileBackend::moveInternal()
 	 */
 	protected function doMoveInternal( array $params ) {
 		// Copy source to dest
@@ -591,32 +612,58 @@ abstract class FileBackend extends FileBackendBase {
 	}
 
 	/**
-	 * @see FileBackend::concatenate()
+	 * @see FileBackend::concatenateInternal()
 	 */
-	abstract protected function doConcatenateInternal( array $params );
+	protected function doConcatenateInternal( array $params ) {
+		$status = Status::newGood();
+		$tmpPath = $params['dst']; // convenience
+
+		// Check that the specified temp file is valid...
+		wfSuppressWarnings();
+		$ok = ( is_file( $tmpPath ) && !filesize( $tmpPath ) );
+		wfRestoreWarnings();
+		if ( !$ok ) { // not present or not empty
+			$status->fatal( 'backend-fail-opentemp', $tmpPath );
+			return $status;
+		}
+
+		// Build up the temp file using the source chunks (in order)...
+		$tmpHandle = fopen( $tmpPath, 'a' );
+		if ( $tmpHandle === false ) {
+			$status->fatal( 'backend-fail-opentemp', $tmpPath );
+			return $status;
+		}
+		foreach ( $params['srcs'] as $virtualSource ) {
+			// Get a local FS version of the chunk
+			$tmpFile = $this->getLocalReference( array( 'src' => $virtualSource ) );
+			if ( !$tmpFile ) {
+				$status->fatal( 'backend-fail-read', $virtualSource );
+				return $status;
+			}
+			// Get a handle to the local FS version
+			$sourceHandle = fopen( $tmpFile->getPath(), 'r' );
+			if ( $sourceHandle === false ) {
+				fclose( $tmpHandle );
+				$status->fatal( 'backend-fail-read', $virtualSource );
+				return $status;
+			}
+			// Append chunk to file (pass chunk size to avoid magic quotes)
+			if ( !stream_copy_to_stream( $sourceHandle, $tmpHandle ) ) {
+				fclose( $sourceHandle );
+				fclose( $tmpHandle );
+				$status->fatal( 'backend-fail-writetemp', $tmpPath );
+				return $status;
+			}
+			fclose( $sourceHandle );
+		}
+		if ( !fclose( $tmpHandle ) ) {
+			$status->fatal( 'backend-fail-closetemp', $tmpPath );
+			return $status;
+		}
 
-	/**
-	 * Create a file in the backend with the given contents.
-	 * Do not call this function from places outside FileBackend and FileOp.
-	 * $params include:
-	 *     content       : the raw file contents
-	 *     dst           : destination storage path
-	 *     overwriteDest : overwrite any file that exists at the destination
-	 * 
-	 * @param $params Array
-	 * @return Status
-	 */
-	final public function createInternal( array $params ) {
-		$status = $this->doCreateInternal( $params );
-		$this->clearCache( array( $params['dst'] ) );
 		return $status;
 	}
 
-	/**
-	 * @see FileBackend::create()
-	 */
-	abstract protected function doCreateInternal( array $params );
-
 	/**
 	 * @see FileBackendBase::prepare()
 	 */
@@ -954,4 +1001,15 @@ abstract class FileBackend extends FileBackendBase {
 	protected function resolveContainerPath( $container, $relStoragePath ) {
 		return $relStoragePath;
 	}
+
+	/**
+	 * Get the final extension from a storage or FS path
+	 * 
+	 * @param $path string
+	 * @return string
+	 */
+	final public static function extensionFromPath( $path ) {
+		$i = strrpos( $path, '.' );
+		return strtolower( $i ? substr( $path, $i + 1 ) : '' );
+	}
 }
diff --git a/includes/filerepo/backend/FileOp.php b/includes/filerepo/backend/FileOp.php
index f6715f1883..38e61ac9a7 100644
--- a/includes/filerepo/backend/FileOp.php
+++ b/includes/filerepo/backend/FileOp.php
@@ -793,19 +793,21 @@ class MoveFileOp extends FileOp {
  * Combines files from severals storage paths into a new file in the backend.
  * Parameters similar to FileBackend::concatenate(), which include:
  *     srcs          : ordered source storage paths (e.g. chunk1, chunk2, ...)
- *     dst           : destination storage path
- *     overwriteDest : do nothing and pass if an identical file exists at destination
+ *     dst           : destination file system path to 0-byte temp file
  */
 class ConcatenateFileOp extends FileOp {
 	protected function allowedParams() {
-		return array( 'srcs', 'dst', 'overwriteDest' );
+		return array( 'srcs', 'dst' );
 	}
 
 	protected function doPrecheck( array &$predicates ) {
 		$status = Status::newGood();
-		// Check if destination file exists
-		$status->merge( $this->precheckDestExistence( $predicates ) );
-		if ( !$status->isOK() ) {
+		// Check destination temp file
+		wfSuppressWarnings();
+		$ok = ( is_file( $this->params['dst'] ) && !filesize( $this->params['dst'] ) );
+		wfRestoreWarnings();
+		if ( !$ok ) { // not present or not empty
+			$status->fatal( 'backend-fail-opentemp', $this->params['dst'] );
 			return $status;
 		}
 		// Check that source files exists
@@ -815,42 +817,31 @@ class ConcatenateFileOp extends FileOp {
 				return $status;
 			}
 		}
-		// Update file existence predicates
-		$predicates['exists'][$this->params['dst']] = true;
 		return $status;
 	}
 
 	protected function doAttempt() {
 		$status = Status::newGood();
-		// Create a destination backup copy as needed
-		if ( $this->destAlreadyExists ) {
-			$status->merge( $this->checkAndBackupDest() );
-			if ( !$status->isOK() ) {
-				return $status;
-			}
-		}
 		// Concatenate the file at the destination
 		$status->merge( $this->backend->concatenateInternal( $this->params ) );
 		return $status;
 	}
 
 	protected function doRevert() {
-		// Restore any file that was at the destination,
-		// overwritting what was put there in attempt()
-		return $this->restoreDest();
-	}
-
-	protected function getSourceSha1Base36() {
-		return null; // defer this until we finish building the new file
+		$status = Status::newGood();
+		// Clear out the temp file back to 0-bytes
+		wfSuppressWarnings();
+		$ok = file_put_contents( $this->params['dst'], '' );
+		wfRestoreWarnings();
+		if ( !$ok ) {
+			$status->fatal( 'backend-fail-writetemp', $this->params['dst'] );
+		}
+		return $status;
 	}
 
 	public function storagePathsRead() {
 		return $this->params['srcs'];
 	}
-
-	public function storagePathsChanged() {
-		return array( $this->params['dst'] );
-	}
 }
 
 /**
diff --git a/includes/filerepo/file/File.php b/includes/filerepo/file/File.php
index a241d6bd8d..4cf15f04a3 100644
--- a/includes/filerepo/file/File.php
+++ b/includes/filerepo/file/File.php
@@ -802,7 +802,7 @@ abstract class File {
 			// Copy any thumbnail from the FS into storage at $dstpath
 			$status = $this->repo->store(
 				$tmpThumbPath, 'thumb', $this->getThumbRel( $thumbName ),
-				FileRepo::OVERWRITE | FileRepo::SKIP_LOCKING );
+				FileRepo::OVERWRITE | FileRepo::SKIP_LOCKING | FileRepo::ALLOW_STALE );
 			if ( !$status->isOK() ) {
 				return new MediaTransformError( 'thumbnail_error',
 					$params['width'], 0, wfMsg( 'thumbnail-dest-create' ) );
diff --git a/includes/upload/UploadFromChunks.php b/includes/upload/UploadFromChunks.php
index 3a5f8099be..651b2a3dc3 100644
--- a/includes/upload/UploadFromChunks.php
+++ b/includes/upload/UploadFromChunks.php
@@ -74,7 +74,7 @@ class UploadFromChunks extends UploadFromFile {
 		
 		$metadata = $this->stash->getMetadata( $key );
 		$this->initializePathInfo( $name,
-			$this->getRealPath ( $metadata['us_path'] ),
+			$this->getRealPath( $metadata['us_path'] ),
 			$metadata['us_size'],
 			false
 		);
@@ -86,8 +86,8 @@ class UploadFromChunks extends UploadFromFile {
 	 */
 	public function concatenateChunks() {
 		wfDebug( __METHOD__ . " concatenate {$this->mChunkIndex} chunks:" . 
-					$this->getOffset() . ' inx:' . $this->getChunkIndex() . "\n" );
-					
+			$this->getOffset() . ' inx:' . $this->getChunkIndex() . "\n" );
+
 		// Concatenate all the chunks to mVirtualTempPath
 		$fileList = Array();
 		// The first chunk is stored at the mVirtualTempPath path so we start on "chunk 1"
@@ -95,13 +95,20 @@ class UploadFromChunks extends UploadFromFile {
 			$fileList[] = $this->getVirtualChunkLocation( $i );
 		}
 
-		// Concatenate into the mVirtualTempPath location;
-		$status = $this->repo->concatenate( $fileList, $this->mVirtualTempPath, FileRepo::DELETE_SOURCE );
+		// 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 );
+		$tmpPath = $tmpFile
+			? $tmpFile->getPath()
+			: false; // fail in concatenate()
+		// Concatenate the chunks at the temp file
+		$status = $this->repo->concatenate( $fileList, $tmpPath, FileRepo::DELETE_SOURCE );
 		if( !$status->isOk() ){
 			return $status; 
 		}
 		// Update the mTempPath variable ( for FileUpload or normal Stash to take over )  
-		$this->mTempPath = $this->getRealPath( $this->mVirtualTempPath );
+		$this->mTempPath = $tmpPath; // file system path
 		return $status;
 	}
 
@@ -115,7 +122,6 @@ class UploadFromChunks extends UploadFromFile {
 	 */
 	public function performUpload( $comment, $pageText, $watch, $user ) {
 		$rv = parent::performUpload( $comment, $pageText, $watch, $user );
-		$this->repo->freeTemp( $this->mVirtualTempPath );
 		return $rv;
 	}
 
diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php
index 043f0a0b56..aea98cecfd 100644
--- a/tests/phpunit/includes/filerepo/FileBackendTest.php
+++ b/tests/phpunit/includes/filerepo/FileBackendTest.php
@@ -349,10 +349,11 @@ class FileBackendTest extends MediaWikiTestCase {
 
 		$dest = $op['dst'];
 		if ( $alreadyExists ) {
-			$oldText = 'blah...blah...waahwaah';
-			$status = $this->backend->doOperation(
-				array( 'op' => 'create', 'content' => $oldText, 'dst' => $dest ) );
-			$this->assertEquals( true, $status->isOK(), "Creation of file at $dest succeeded." );
+			$ok = file_put_contents( $dest, 'blah...blah...waahwaah' ) !== false;
+			$this->assertEquals( true, $ok, "Creation of file at $dest succeeded." );
+		} else {
+			$ok = file_put_contents( $dest, '' ) !== false;
+			$this->assertEquals( true, $ok, "Creation of 0-byte file at $dest succeeded." );
 		}
 
 		// Combine them
@@ -364,18 +365,15 @@ class FileBackendTest extends MediaWikiTestCase {
 		}
 
 		if ( $okStatus ) {
-			$this->assertEquals( true, $this->backend->fileExists( array( 'src' => $dest ) ),
+			$this->assertEquals( true, is_file( $dest ),
 				"Dest concat file $dest exists after creation." );
 		} else {
-			$this->assertEquals( true, $this->backend->fileExists( array( 'src' => $dest ) ),
+			$this->assertEquals( true, is_file( $dest ),
 				"Dest concat file $dest exists after failed creation." );
 		}
 
-		$tmpFile = $this->backend->getLocalCopy( array( 'src' => $dest ) );
-		$this->assertNotNull( $tmpFile, "Creation of local copy of $dest succeeded." );
-
-		$contents = file_get_contents( $tmpFile->getPath() );
-		$this->assertNotEquals( false, $contents, "Local copy of $dest exists." );
+		$contents = file_get_contents( $dest );
+		$this->assertNotEquals( false, $contents, "File at $dest exists." );
 
 		if ( $okStatus ) {
 			$this->assertEquals( $expContent, $contents, "Concat file at $dest has correct contents." );
@@ -387,7 +385,8 @@ class FileBackendTest extends MediaWikiTestCase {
 	function provider_testConcatenate() {
 		$cases = array();
 
-		$dest = $this->singleBasePath() . '/cont1/full_file.txt';
+		$rand = mt_rand( 0, 2000000000 ) . time();
+		$dest = wfTempDir() . "/randomfile!$rand.txt";
 		$srcs = array(
 			$this->singleBasePath() . '/cont1/file1.txt',
 			$this->singleBasePath() . '/cont1/file2.txt',
@@ -430,15 +429,6 @@ class FileBackendTest extends MediaWikiTestCase {
 			false, // succeeds
 		);
 
-		$op['overwriteDest'] = true;
-		$cases[] = array(
-			$op, // operation
-			$srcs, // sources
-			$content, // content for each source
-			true, // no dest already exists
-			true, // succeeds
-		);
-
 		return $cases;
 	}