In FileBackend:

* Made secure() call doPrepare() for caller dummy proofing (especially those that don't check the status).
In FSFileBackend:
* Removed redundant wfMkdirParents() calls in FSFileBackend, we already have prepare() for this purpose. This also keeps it's behavior more consistent with the other backends.
* Made use of 'backend-fail-store' message in doStoreInternal().
Other:
* Updated unit tests and renamed $src => $source in some functions for consistency.
* Added some documentation comments and @since tags.

diff --git a/includes/filerepo/backend/FSFileBackend.php b/includes/filerepo/backend/FSFileBackend.php
index fbf1017..007d155 100644 (file)
--- a/includes/filerepo/backend/FSFileBackend.php
+++ b/includes/filerepo/backend/FSFileBackend.php
@@ -19,6 +19,7 @@
  * Likewise, error suppression should be used to avoid path disclosure.
  *
  * @ingroup FileBackend
+ * @since 1.19
  */
 class FSFileBackend extends FileBackend {
        protected $basePath; // string; directory holding the container directories
@@ -125,18 +126,13 @@ class FSFileBackend extends FileBackend {
                                $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
                                return $status;
                        }
-               } else {
-                       if ( !wfMkdirParents( dirname( $dest ) ) ) {
-                               $status->fatal( 'directorycreateerror', $params['dst'] );
-                               return $status;
-                       }
                }
 
                wfSuppressWarnings();
                $ok = copy( $params['src'], $dest );
                wfRestoreWarnings();
                if ( !$ok ) {
-                       $status->fatal( 'backend-fail-copy', $params['src'], $params['dst'] );
+                       $status->fatal( 'backend-fail-store', $params['src'], $params['dst'] );
                        return $status;
                }
 
@@ -176,11 +172,6 @@ class FSFileBackend extends FileBackend {
                                $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
                                return $status;
                        }
-               } else {
-                       if ( !wfMkdirParents( dirname( $dest ) ) ) {
-                               $status->fatal( 'directorycreateerror', $params['dst'] );
-                               return $status;
-                       }
                }
 
                wfSuppressWarnings();
@@ -230,11 +221,6 @@ class FSFileBackend extends FileBackend {
                                $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
                                return $status;
                        }
-               } else {
-                       if ( !wfMkdirParents( dirname( $dest ) ) ) {
-                               $status->fatal( 'directorycreateerror', $params['dst'] );
-                               return $status;
-                       }
                }
 
                wfSuppressWarnings();
@@ -304,11 +290,6 @@ class FSFileBackend extends FileBackend {
                                $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
                                return $status;
                        }
-               } else {
-                       if ( !wfMkdirParents( dirname( $dest ) ) ) {
-                               $status->fatal( 'directorycreateerror', $params['dst'] );
-                               return $status;
-                       }
                }
 
                wfSuppressWarnings();
@@ -332,7 +313,7 @@ class FSFileBackend extends FileBackend {
                list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] );
                $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid
                $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot;
-               if ( !wfMkdirParents( $dir ) ) {
+               if ( !wfMkdirParents( $dir ) ) { // make directory and its parents
                        $status->fatal( 'directorycreateerror', $params['dir'] );
                } elseif ( !is_writable( $dir ) ) {
                        $status->fatal( 'directoryreadonlyerror', $params['dir'] );
@@ -350,10 +331,6 @@ class FSFileBackend extends FileBackend {
                list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] );
                $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid
                $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot;
-               if ( !wfMkdirParents( $dir ) ) {
-                       $status->fatal( 'directorycreateerror', $params['dir'] );
-                       return $status;
-               }
                // Seed new directories with a blank index.html, to prevent crawling...
                if ( !empty( $params['noListing'] ) && !file_exists( "{$dir}/index.html" ) ) {
                        wfSuppressWarnings();
@@ -527,6 +504,7 @@ class FSFileBackend extends FileBackend {
 /**
  * Wrapper around RecursiveDirectoryIterator that catches
  * exception or does any custom behavoir that we may want.
+ * Do not use this class from places outside FSFileBackend.
  *
  * @ingroup FileBackend
  */

diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php
index 7af34ef..267bc5e 100644 (file)
--- a/includes/filerepo/backend/FileBackend.php
+++ b/includes/filerepo/backend/FileBackend.php
@@ -323,7 +323,11 @@ abstract class FileBackendBase {
                if ( $this->readOnly != '' ) {
                        return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly );
                }
-               return $this->doSecure( $params );
+               $status = $this->doPrepare( $params ); // dir must exist to restrict it
+               if ( $status->isOK() ) {
+                       $status->merge( $this->doSecure( $params ) );
+               }
+               return $status;
        }
 
        /**

diff --git a/includes/filerepo/backend/FileBackendGroup.php b/includes/filerepo/backend/FileBackendGroup.php
index 24277b1..cb4ef31 100644 (file)
--- a/includes/filerepo/backend/FileBackendGroup.php
+++ b/includes/filerepo/backend/FileBackendGroup.php
@@ -9,6 +9,7 @@
  * Class to handle file backend registration
  *
  * @ingroup FileBackend
+ * @since 1.19
  */
 class FileBackendGroup {
        protected static $instance = null;

diff --git a/includes/filerepo/backend/FileBackendMultiWrite.php b/includes/filerepo/backend/FileBackendMultiWrite.php
index 4258d13..8685774 100644 (file)
--- a/includes/filerepo/backend/FileBackendMultiWrite.php
+++ b/includes/filerepo/backend/FileBackendMultiWrite.php
@@ -18,6 +18,7 @@
  * If an operation fails on one backend it will be rolled back from the others.
  *
  * @ingroup FileBackend
+ * @since 1.19
  */
 class FileBackendMultiWrite extends FileBackendBase {
        /** @var Array Prioritized list of FileBackend objects */

diff --git a/includes/filerepo/backend/FileOp.php b/includes/filerepo/backend/FileOp.php
index 11fe865..492a0e7 100644 (file)
--- a/includes/filerepo/backend/FileOp.php
+++ b/includes/filerepo/backend/FileOp.php
@@ -8,8 +8,9 @@
 /**
  * Helper class for representing operations with transaction support.
  * FileBackend::doOperations() will require these classes for supported operations.
+ * Do not use this class from places outside FileBackend.
  * 
- * Use of large fields should be avoided as we want to be able to support
+ * Use of large fields should be avoided as we want to support
  * potentially many FileOp classes in large arrays in memory.
  * 
  * @ingroup FileBackend

diff --git a/includes/filerepo/backend/lockmanager/DBLockManager.php b/includes/filerepo/backend/lockmanager/DBLockManager.php
index ac9c079..900a639 100644 (file)
--- a/includes/filerepo/backend/lockmanager/DBLockManager.php
+++ b/includes/filerepo/backend/lockmanager/DBLockManager.php
@@ -14,6 +14,7 @@
  * Caching is used to avoid hitting servers that are down.
  *
  * @ingroup LockManager
+ * @since 1.19
  */
 class DBLockManager extends LockManager {
        /** @var Array Map of DB names to server config */

diff --git a/includes/filerepo/backend/lockmanager/FSLockManager.php b/includes/filerepo/backend/lockmanager/FSLockManager.php
index be5fda9..33aceb4 100644 (file)
--- a/includes/filerepo/backend/lockmanager/FSLockManager.php
+++ b/includes/filerepo/backend/lockmanager/FSLockManager.php
@@ -10,6 +10,7 @@
  * locks will be ignored; see http://nfs.sourceforge.net/#section_d.
  *
  * @ingroup LockManager
+ * @since 1.19
  */
 class FSLockManager extends LockManager {
        /** @var Array Mapping of lock types to the type actually used */

diff --git a/includes/filerepo/backend/lockmanager/LSLockManager.php b/includes/filerepo/backend/lockmanager/LSLockManager.php
index 383c77c..58fb8f3 100644 (file)
--- a/includes/filerepo/backend/lockmanager/LSLockManager.php
+++ b/includes/filerepo/backend/lockmanager/LSLockManager.php
@@ -11,6 +11,7 @@
  * A majority of peers must agree for a lock to be acquired.
  *
  * @ingroup LockManager
+ * @since 1.19
  */
 class LSLockManager extends LockManager {
        /** @var Array Mapping of lock types to the type actually used */

diff --git a/includes/filerepo/backend/lockmanager/LockManager.php b/includes/filerepo/backend/lockmanager/LockManager.php
index eb47623..c197976 100644 (file)
--- a/includes/filerepo/backend/lockmanager/LockManager.php
+++ b/includes/filerepo/backend/lockmanager/LockManager.php
@@ -161,6 +161,7 @@ class ScopedLock {
 
 /**
  * Simple version of LockManager that does nothing
+ * @since 1.19
  */
 class NullLockManager extends LockManager {
        protected function doLock( array $paths, $type ) {

diff --git a/includes/filerepo/backend/lockmanager/LockManagerGroup.php b/includes/filerepo/backend/lockmanager/LockManagerGroup.php
index e501a0f..3fa91d5 100644 (file)
--- a/includes/filerepo/backend/lockmanager/LockManagerGroup.php
+++ b/includes/filerepo/backend/lockmanager/LockManagerGroup.php
@@ -4,6 +4,7 @@
  * 
  * @ingroup LockManager
  * @author Aaron Schulz
+ * @since 1.19
  */
 class LockManagerGroup {
        protected static $instance = null;

diff --git a/tests/phpunit/includes/filerepo/FileBackendTest.php b/tests/phpunit/includes/filerepo/FileBackendTest.php
index 59fa1ab..cffa7e3 100644 (file)
--- a/tests/phpunit/includes/filerepo/FileBackendTest.php
+++ b/tests/phpunit/includes/filerepo/FileBackendTest.php
@@ -72,6 +72,8 @@ class FileBackendTest extends MediaWikiTestCase {
        function doTestStore( $op, $source, $dest ) {
                $backendName = $this->backendClass();
 
+               $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
+
                file_put_contents( $source, "Unit test file" );
                $status = $this->backend->doOperation( $op );
 
@@ -137,6 +139,9 @@ class FileBackendTest extends MediaWikiTestCase {
        function doTestCopy( $op, $source, $dest ) {
                $backendName = $this->backendClass();
 
+               $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
+               $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
+
                $status = $this->backend->doOperation(
                        array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
                $this->assertEquals( true, $status->isOK(),
@@ -207,6 +212,9 @@ class FileBackendTest extends MediaWikiTestCase {
        public function doTestMove( $op, $source, $dest ) {
                $backendName = $this->backendClass();
 
+               $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
+               $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
+
                $status = $this->backend->doOperation(
                        array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
                $this->assertEquals( true, $status->isOK(),
@@ -278,6 +286,8 @@ class FileBackendTest extends MediaWikiTestCase {
        public function doTestDelete( $op, $source, $withSource, $okStatus ) {
                $backendName = $this->backendClass();
 
+               $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
+
                if ( $withSource ) {
                        $status = $this->backend->doOperation(
                                array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
@@ -359,6 +369,8 @@ class FileBackendTest extends MediaWikiTestCase {
        public function doTestCreate( $op, $dest, $alreadyExists, $okStatus, $newSize ) {
                $backendName = $this->backendClass();
 
+               $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
+
                $oldText = 'blah...blah...waahwaah';
                if ( $alreadyExists ) {
                        $status = $this->backend->doOperation(
@@ -462,6 +474,7 @@ class FileBackendTest extends MediaWikiTestCase {
                // Create sources
                $ops = array();
                foreach ( $srcs as $i => $source ) {
+                       $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
                        $ops[] = array(
                                'op'      => 'create', // operation
                                'dst'     => $source, // source
@@ -585,20 +598,22 @@ class FileBackendTest extends MediaWikiTestCase {
        /**
         * @dataProvider provider_testGetFileContents
         */
-       public function doTestGetFileContents( $src, $content ) {
+       public function doTestGetFileContents( $source, $content ) {
                $backendName = $this->backendClass();
 
+               $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
+
                $status = $this->backend->doOperation(
-                       array( 'op' => 'create', 'content' => $content, 'dst' => $src ) );
+                       array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
                $this->assertEquals( true, $status->isOK(),
-                       "Creation of file at $src succeeded ($backendName)." );
+                       "Creation of file at $source succeeded ($backendName)." );
 
-               $newContents = $this->backend->getFileContents( array( 'src' => $src ) );
+               $newContents = $this->backend->getFileContents( array( 'src' => $source ) );
                $this->assertNotEquals( false, $newContents,
-                       "Read of file at $src succeeded ($backendName)." );
+                       "Read of file at $source succeeded ($backendName)." );
 
                $this->assertEquals( $content, $newContents,
-                       "Contents read match data at $src ($backendName)." );
+                       "Contents read match data at $source ($backendName)." );
        }
 
        function provider_testGetFileContents() {
@@ -626,20 +641,22 @@ class FileBackendTest extends MediaWikiTestCase {
                $this->tearDownFiles();
        }
 
-       public function doTestGetLocalCopy( $src, $content ) {
+       public function doTestGetLocalCopy( $source, $content ) {
                $backendName = $this->backendClass();
 
+               $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
+
                $status = $this->backend->doOperation(
-                       array( 'op' => 'create', 'content' => $content, 'dst' => $src ) );
+                       array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
                $this->assertEquals( true, $status->isOK(),
-                       "Creation of file at $src succeeded ($backendName)." );
+                       "Creation of file at $source succeeded ($backendName)." );
 
-               $tmpFile = $this->backend->getLocalCopy( array( 'src' => $src ) );
+               $tmpFile = $this->backend->getLocalCopy( array( 'src' => $source ) );
                $this->assertNotNull( $tmpFile,
-                       "Creation of local copy of $src succeeded ($backendName)." );
+                       "Creation of local copy of $source succeeded ($backendName)." );
 
                $contents = file_get_contents( $tmpFile->getPath() );
-               $this->assertNotEquals( false, $contents, "Local copy of $src exists ($backendName)." );
+               $this->assertNotEquals( false, $contents, "Local copy of $source exists ($backendName)." );
        }
 
        function provider_testGetLocalCopy() {
@@ -667,20 +684,22 @@ class FileBackendTest extends MediaWikiTestCase {
                $this->tearDownFiles();
        }
 
-       public function doTestGetLocalReference( $src, $content ) {
+       public function doTestGetLocalReference( $source, $content ) {
                $backendName = $this->backendClass();
 
+               $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
+
                $status = $this->backend->doOperation(
-                       array( 'op' => 'create', 'content' => $content, 'dst' => $src ) );
+                       array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
                $this->assertEquals( true, $status->isOK(),
-                       "Creation of file at $src succeeded ($backendName)." );
+                       "Creation of file at $source succeeded ($backendName)." );
 
-               $tmpFile = $this->backend->getLocalReference( array( 'src' => $src ) );
+               $tmpFile = $this->backend->getLocalReference( array( 'src' => $source ) );
                $this->assertNotNull( $tmpFile,
-                       "Creation of local copy of $src succeeded ($backendName)." );
+                       "Creation of local copy of $source succeeded ($backendName)." );
 
                $contents = file_get_contents( $tmpFile->getPath() );
-               $this->assertNotEquals( false, $contents, "Local copy of $src exists ($backendName)." );
+               $this->assertNotEquals( false, $contents, "Local copy of $source exists ($backendName)." );
        }
 
        function provider_testGetLocalReference() {
@@ -779,6 +798,7 @@ class FileBackendTest extends MediaWikiTestCase {
                $ops = array();
                foreach ( $files as $file ) {
                        $ops[] = array( 'op' => 'create', 'content' => 'xxy', 'dst' => $file );
+                       $this->backend->prepare( array( 'dir' => dirname( $file ) ) );
                }
                $status = $this->backend->doOperations( $ops );
                $this->assertEquals( true, $status->isOK(),