r108878 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r108877‎ | r108878 | r108879 >
Date:23:30, 13 January 2012
Author:aaron
Status:ok
Tags:filebackend 
Comment:
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.
Modified paths:
  • /trunk/phase3/includes/filerepo/backend/FSFileBackend.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileBackend.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileBackendGroup.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileBackendMultiWrite.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileOp.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/lockmanager/DBLockManager.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/lockmanager/FSLockManager.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/lockmanager/LSLockManager.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/lockmanager/LockManager.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/lockmanager/LockManagerGroup.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php
——@@ -72,6 +72,8 @@
7373 function doTestStore( $op, $source, $dest ) {
7474 $backendName = $this->backendClass();
7575
 76+ $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
 77+
7678 file_put_contents( $source, "Unit test file" );
7779 $status = $this->backend->doOperation( $op );
7880
——@@ -137,6 +139,9 @@
138140 function doTestCopy( $op, $source, $dest ) {
139141 $backendName = $this->backendClass();
140142
 143+ $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 144+ $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
 145+
141146 $status = $this->backend->doOperation(
142147 array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
143148 $this->assertEquals( true, $status->isOK(),
——@@ -207,6 +212,9 @@
208213 public function doTestMove( $op, $source, $dest ) {
209214 $backendName = $this->backendClass();
210215
 216+ $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 217+ $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
 218+
211219 $status = $this->backend->doOperation(
212220 array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
213221 $this->assertEquals( true, $status->isOK(),
——@@ -278,6 +286,8 @@
279287 public function doTestDelete( $op, $source, $withSource, $okStatus ) {
280288 $backendName = $this->backendClass();
281289
 290+ $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 291+
282292 if ( $withSource ) {
283293 $status = $this->backend->doOperation(
284294 array( 'op' => 'create', 'content' => 'blahblah', 'dst' => $source ) );
——@@ -359,6 +369,8 @@
360370 public function doTestCreate( $op, $dest, $alreadyExists, $okStatus, $newSize ) {
361371 $backendName = $this->backendClass();
362372
 373+ $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
 374+
363375 $oldText = 'blah...blah...waahwaah';
364376 if ( $alreadyExists ) {
365377 $status = $this->backend->doOperation(
——@@ -462,6 +474,7 @@
463475 // Create sources
464476 $ops = array();
465477 foreach ( $srcs as $i => $source ) {
 478+ $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
466479 $ops[] = array(
467480 'op' => 'create', // operation
468481 'dst' => $source, // source
——@@ -585,20 +598,22 @@
586599 /**
587600 * @dataProvider provider_testGetFileContents
588601 */
589 - public function doTestGetFileContents( $src, $content ) {
 602+ public function doTestGetFileContents( $source, $content ) {
590603 $backendName = $this->backendClass();
591604
 605+ $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 606+
592607 $status = $this->backend->doOperation(
593 - array( 'op' => 'create', 'content' => $content, 'dst' => $src ) );
 608+ array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
594609 $this->assertEquals( true, $status->isOK(),
595 - "Creation of file at $src succeeded ($backendName)." );
 610+ "Creation of file at $source succeeded ($backendName)." );
596611
597 - $newContents = $this->backend->getFileContents( array( 'src' => $src ) );
 612+ $newContents = $this->backend->getFileContents( array( 'src' => $source ) );
598613 $this->assertNotEquals( false, $newContents,
599 - "Read of file at $src succeeded ($backendName)." );
 614+ "Read of file at $source succeeded ($backendName)." );
600615
601616 $this->assertEquals( $content, $newContents,
602 - "Contents read match data at $src ($backendName)." );
 617+ "Contents read match data at $source ($backendName)." );
603618 }
604619
605620 function provider_testGetFileContents() {
——@@ -626,20 +641,22 @@
627642 $this->tearDownFiles();
628643 }
629644
630 - public function doTestGetLocalCopy( $src, $content ) {
 645+ public function doTestGetLocalCopy( $source, $content ) {
631646 $backendName = $this->backendClass();
632647
 648+ $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 649+
633650 $status = $this->backend->doOperation(
634 - array( 'op' => 'create', 'content' => $content, 'dst' => $src ) );
 651+ array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
635652 $this->assertEquals( true, $status->isOK(),
636 - "Creation of file at $src succeeded ($backendName)." );
 653+ "Creation of file at $source succeeded ($backendName)." );
637654
638 - $tmpFile = $this->backend->getLocalCopy( array( 'src' => $src ) );
 655+ $tmpFile = $this->backend->getLocalCopy( array( 'src' => $source ) );
639656 $this->assertNotNull( $tmpFile,
640 - "Creation of local copy of $src succeeded ($backendName)." );
 657+ "Creation of local copy of $source succeeded ($backendName)." );
641658
642659 $contents = file_get_contents( $tmpFile->getPath() );
643 - $this->assertNotEquals( false, $contents, "Local copy of $src exists ($backendName)." );
 660+ $this->assertNotEquals( false, $contents, "Local copy of $source exists ($backendName)." );
644661 }
645662
646663 function provider_testGetLocalCopy() {
——@@ -667,20 +684,22 @@
668685 $this->tearDownFiles();
669686 }
670687
671 - public function doTestGetLocalReference( $src, $content ) {
 688+ public function doTestGetLocalReference( $source, $content ) {
672689 $backendName = $this->backendClass();
673690
 691+ $this->backend->prepare( array( 'dir' => dirname( $source ) ) );
 692+
674693 $status = $this->backend->doOperation(
675 - array( 'op' => 'create', 'content' => $content, 'dst' => $src ) );
 694+ array( 'op' => 'create', 'content' => $content, 'dst' => $source ) );
676695 $this->assertEquals( true, $status->isOK(),
677 - "Creation of file at $src succeeded ($backendName)." );
 696+ "Creation of file at $source succeeded ($backendName)." );
678697
679 - $tmpFile = $this->backend->getLocalReference( array( 'src' => $src ) );
 698+ $tmpFile = $this->backend->getLocalReference( array( 'src' => $source ) );
680699 $this->assertNotNull( $tmpFile,
681 - "Creation of local copy of $src succeeded ($backendName)." );
 700+ "Creation of local copy of $source succeeded ($backendName)." );
682701
683702 $contents = file_get_contents( $tmpFile->getPath() );
684 - $this->assertNotEquals( false, $contents, "Local copy of $src exists ($backendName)." );
 703+ $this->assertNotEquals( false, $contents, "Local copy of $source exists ($backendName)." );
685704 }
686705
687706 function provider_testGetLocalReference() {
——@@ -779,6 +798,7 @@
780799 $ops = array();
781800 foreach ( $files as $file ) {
782801 $ops[] = array( 'op' => 'create', 'content' => 'xxy', 'dst' => $file );
 802+ $this->backend->prepare( array( 'dir' => dirname( $file ) ) );
783803 }
784804 $status = $this->backend->doOperations( $ops );
785805 $this->assertEquals( true, $status->isOK(),
Index: trunk/phase3/includes/filerepo/backend/FileBackendMultiWrite.php
——@@ -18,6 +18,7 @@
1919 * If an operation fails on one backend it will be rolled back from the others.
2020 *
2121 * @ingroup FileBackend
 22+ * @since 1.19
2223 */
2324 class FileBackendMultiWrite extends FileBackendBase {
2425 /** @var Array Prioritized list of FileBackend objects */
Index: trunk/phase3/includes/filerepo/backend/lockmanager/LockManagerGroup.php
——@@ -4,6 +4,7 @@
55 *
66 * @ingroup LockManager
77 * @author Aaron Schulz
 8+ * @since 1.19
89 */
910 class LockManagerGroup {
1011 protected static $instance = null;
Index: trunk/phase3/includes/filerepo/backend/lockmanager/LSLockManager.php
——@@ -11,6 +11,7 @@
1212 * A majority of peers must agree for a lock to be acquired.
1313 *
1414 * @ingroup LockManager
 15+ * @since 1.19
1516 */
1617 class LSLockManager extends LockManager {
1718 /** @var Array Mapping of lock types to the type actually used */
Index: trunk/phase3/includes/filerepo/backend/lockmanager/LockManager.php
——@@ -161,6 +161,7 @@
162162
163163 /**
164164 * Simple version of LockManager that does nothing
 165+ * @since 1.19
165166 */
166167 class NullLockManager extends LockManager {
167168 protected function doLock( array $paths, $type ) {
Index: trunk/phase3/includes/filerepo/backend/lockmanager/DBLockManager.php
——@@ -14,6 +14,7 @@
1515 * Caching is used to avoid hitting servers that are down.
1616 *
1717 * @ingroup LockManager
 18+ * @since 1.19
1819 */
1920 class DBLockManager extends LockManager {
2021 /** @var Array Map of DB names to server config */
Index: trunk/phase3/includes/filerepo/backend/lockmanager/FSLockManager.php
——@@ -10,6 +10,7 @@
1111 * locks will be ignored; see http://nfs.sourceforge.net/#section_d.
1212 *
1313 * @ingroup LockManager
 14+ * @since 1.19
1415 */
1516 class FSLockManager extends LockManager {
1617 /** @var Array Mapping of lock types to the type actually used */
Index: trunk/phase3/includes/filerepo/backend/FileOp.php
——@@ -8,8 +8,9 @@
99 /**
1010 * Helper class for representing operations with transaction support.
1111 * FileBackend::doOperations() will require these classes for supported operations.
 12+ * Do not use this class from places outside FileBackend.
1213 *
13 - * Use of large fields should be avoided as we want to be able to support
 14+ * Use of large fields should be avoided as we want to support
1415 * potentially many FileOp classes in large arrays in memory.
1516 *
1617 * @ingroup FileBackend
Index: trunk/phase3/includes/filerepo/backend/FSFileBackend.php
——@@ -19,6 +19,7 @@
2020 * Likewise, error suppression should be used to avoid path disclosure.
2121 *
2222 * @ingroup FileBackend
 23+ * @since 1.19
2324 */
2425 class FSFileBackend extends FileBackend {
2526 protected $basePath; // string; directory holding the container directories
——@@ -125,18 +126,13 @@
126127 $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
127128 return $status;
128129 }
129 - } else {
130 - if ( !wfMkdirParents( dirname( $dest ) ) ) {
131 - $status->fatal( 'directorycreateerror', $params['dst'] );
132 - return $status;
133 - }
134130 }
135131
136132 wfSuppressWarnings();
137133 $ok = copy( $params['src'], $dest );
138134 wfRestoreWarnings();
139135 if ( !$ok ) {
140 - $status->fatal( 'backend-fail-copy', $params['src'], $params['dst'] );
 136+ $status->fatal( 'backend-fail-store', $params['src'], $params['dst'] );
141137 return $status;
142138 }
143139
——@@ -176,11 +172,6 @@
177173 $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
178174 return $status;
179175 }
180 - } else {
181 - if ( !wfMkdirParents( dirname( $dest ) ) ) {
182 - $status->fatal( 'directorycreateerror', $params['dst'] );
183 - return $status;
184 - }
185176 }
186177
187178 wfSuppressWarnings();
——@@ -230,11 +221,6 @@
231222 $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
232223 return $status;
233224 }
234 - } else {
235 - if ( !wfMkdirParents( dirname( $dest ) ) ) {
236 - $status->fatal( 'directorycreateerror', $params['dst'] );
237 - return $status;
238 - }
239225 }
240226
241227 wfSuppressWarnings();
——@@ -304,11 +290,6 @@
305291 $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
306292 return $status;
307293 }
308 - } else {
309 - if ( !wfMkdirParents( dirname( $dest ) ) ) {
310 - $status->fatal( 'directorycreateerror', $params['dst'] );
311 - return $status;
312 - }
313294 }
314295
315296 wfSuppressWarnings();
——@@ -332,7 +313,7 @@
333314 list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] );
334315 $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid
335316 $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot;
336 - if ( !wfMkdirParents( $dir ) ) {
 317+ if ( !wfMkdirParents( $dir ) ) { // make directory and its parents
337318 $status->fatal( 'directorycreateerror', $params['dir'] );
338319 } elseif ( !is_writable( $dir ) ) {
339320 $status->fatal( 'directoryreadonlyerror', $params['dir'] );
——@@ -350,10 +331,6 @@
351332 list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] );
352333 $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid
353334 $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot;
354 - if ( !wfMkdirParents( $dir ) ) {
355 - $status->fatal( 'directorycreateerror', $params['dir'] );
356 - return $status;
357 - }
358335 // Seed new directories with a blank index.html, to prevent crawling...
359336 if ( !empty( $params['noListing'] ) && !file_exists( "{$dir}/index.html" ) ) {
360337 wfSuppressWarnings();
——@@ -527,6 +504,7 @@
528505 /**
529506 * Wrapper around RecursiveDirectoryIterator that catches
530507 * exception or does any custom behavoir that we may want.
 508+ * Do not use this class from places outside FSFileBackend.
531509 *
532510 * @ingroup FileBackend
533511 */
Index: trunk/phase3/includes/filerepo/backend/FileBackendGroup.php
——@@ -9,6 +9,7 @@
1010 * Class to handle file backend registration
1111 *
1212 * @ingroup FileBackend
 13+ * @since 1.19
1314 */
1415 class FileBackendGroup {
1516 protected static $instance = null;
Index: trunk/phase3/includes/filerepo/backend/FileBackend.php
——@@ -323,7 +323,11 @@
324324 if ( $this->readOnly != '' ) {
325325 return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly );
326326 }
327 - return $this->doSecure( $params );
 327+ $status = $this->doPrepare( $params ); // dir must exist to restrict it
 328+ if ( $status->isOK() ) {
 329+ $status->merge( $this->doSecure( $params ) );
 330+ }
 331+ return $status;
328332 }
329333
330334 /**

Status & tagging log