r108377 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r108376‎ | r108377 | r108378 >
Date:00:20, 9 January 2012
Author:aaron
Status:ok
Tags:
Comment:
* r107986: Added readOnly checks to prepare(), secure(), and clean() in FileBackendBase.
* Added some prepare()/clean() tests.
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/FileBackendMultiWrite.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php
——@@ -690,12 +690,54 @@
691691 return $cases;
692692 }
693693
694 - // @TODO: testPrepare
 694+ /**
 695+ * @dataProvider provider_testPrepareAndClean
 696+ */
 697+ public function testPrepareAndClean( $path, $isOK ) {
 698+ $this->backend = $this->singleBackend;
 699+ $this->doTestPrepareAndClean( $path, $isOK );
695700
 701+ $this->backend = $this->multiBackend;
 702+ $this->doTestPrepareAndClean( $path, $isOK );
 703+ }
 704+
 705+ function provider_testPrepareAndClean() {
 706+ $base = $this->baseStorePath();
 707+ return array(
 708+ array( "$base/cont1/a/z/some_file1.txt", true ),
 709+ array( "$base/cont2/a/z/some_file2.txt", true ),
 710+ array( "$base/cont3/a/z/some_file3.txt", false ),
 711+ );
 712+ }
 713+
 714+ function doTestPrepareAndClean( $path, $isOK ) {
 715+ $backendName = $this->backendClass();
 716+
 717+ $status = $this->backend->prepare( array( 'dir' => $path ) );
 718+ if ( $isOK ) {
 719+ $this->assertEquals( array(), $status->errors,
 720+ "Preparing dir $path succeeded without warnings ($backendName)." );
 721+ $this->assertEquals( true, $status->isOK(),
 722+ "Preparing dir $path succeeded ($backendName)." );
 723+ } else {
 724+ $this->assertEquals( false, $status->isOK(),
 725+ "Preparing dir $path failed ($backendName)." );
 726+ }
 727+
 728+ $status = $this->backend->clean( array( 'dir' => $path ) );
 729+ if ( $isOK ) {
 730+ $this->assertEquals( array(), $status->errors,
 731+ "Cleaning dir $path succeeded without warnings ($backendName)." );
 732+ $this->assertEquals( true, $status->isOK(),
 733+ "Cleaning dir $path succeeded ($backendName)." );
 734+ } else {
 735+ $this->assertEquals( false, $status->isOK(),
 736+ "Cleaning dir $path failed ($backendName)." );
 737+ }
 738+ }
 739+
696740 // @TODO: testSecure
697741
698 - // @TODO: testClean
699 -
700742 // @TODO: testDoOperations
701743
702744 public function testGetFileList() {
Index: trunk/phase3/includes/filerepo/backend/FileBackendMultiWrite.php
——@@ -251,37 +251,37 @@
252252 }
253253
254254 /**
255 - * @see FileBackendBase::prepare()
 255+ * @see FileBackendBase::doPrepare()
256256 */
257 - public function prepare( array $params ) {
 257+ public function doPrepare( array $params ) {
258258 $status = Status::newGood();
259259 foreach ( $this->backends as $backend ) {
260260 $realParams = $this->substOpPaths( $params, $backend );
261 - $status->merge( $backend->prepare( $realParams ) );
 261+ $status->merge( $backend->doPrepare( $realParams ) );
262262 }
263263 return $status;
264264 }
265265
266266 /**
267 - * @see FileBackendBase::secure()
 267+ * @see FileBackendBase::doSecure()
268268 */
269 - public function secure( array $params ) {
 269+ public function doSecure( array $params ) {
270270 $status = Status::newGood();
271271 foreach ( $this->backends as $backend ) {
272272 $realParams = $this->substOpPaths( $params, $backend );
273 - $status->merge( $backend->secure( $realParams ) );
 273+ $status->merge( $backend->doSecure( $realParams ) );
274274 }
275275 return $status;
276276 }
277277
278278 /**
279 - * @see FileBackendBase::clean()
 279+ * @see FileBackendBase::doClean()
280280 */
281 - public function clean( array $params ) {
 281+ public function doClean( array $params ) {
282282 $status = Status::newGood();
283283 foreach ( $this->backends as $backend ) {
284284 $realParams = $this->substOpPaths( $params, $backend );
285 - $status->merge( $backend->clean( $realParams ) );
 285+ $status->merge( $backend->doClean( $realParams ) );
286286 }
287287 return $status;
288288 }
Index: trunk/phase3/includes/filerepo/backend/FSFileBackend.php
——@@ -274,9 +274,9 @@
275275 }
276276
277277 /**
278 - * @see FileBackend::doPrepare()
 278+ * @see FileBackend::doPrepareInternal()
279279 */
280 - protected function doPrepare( $container, $dir, array $params ) {
 280+ protected function doPrepareInternal( $container, $dir, array $params ) {
281281 $status = Status::newGood();
282282 if ( !wfMkdirParents( $dir ) ) {
283283 $status->fatal( 'directorycreateerror', $params['dir'] );
——@@ -289,9 +289,9 @@
290290 }
291291
292292 /**
293 - * @see FileBackend::doSecure()
 293+ * @see FileBackend::doSecureInternal()
294294 */
295 - protected function doSecure( $container, $dir, array $params ) {
 295+ protected function doSecureInternal( $container, $dir, array $params ) {
296296 $status = Status::newGood();
297297 if ( !wfMkdirParents( $dir ) ) {
298298 $status->fatal( 'directorycreateerror', $params['dir'] );
——@@ -308,25 +308,27 @@
309309 }
310310 }
311311 // Add a .htaccess file to the root of the container...
312 - list( $b, $container, $r ) = FileBackend::splitStoragePath( $params['dir'] );
313 - $dirRoot = $this->containerPaths[$container]; // real path
314 - if ( !empty( $params['noAccess'] ) && !file_exists( "{$dirRoot}/.htaccess" ) ) {
315 - wfSuppressWarnings();
316 - $ok = file_put_contents( "{$dirRoot}/.htaccess", "Deny from all\n" );
317 - wfRestoreWarnings();
318 - if ( !$ok ) {
319 - $storeDir = "mwstore://{$this->name}/{$container}";
320 - $status->fatal( 'backend-fail-create', "$storeDir/.htaccess" );
321 - return $status;
 312+ if ( !empty( $params['noAccess'] ) ) {
 313+ list( $b, $container, $r ) = FileBackend::splitStoragePath( $params['dir'] );
 314+ $dirRoot = $this->containerPaths[$container]; // real path
 315+ if ( !file_exists( "{$dirRoot}/.htaccess" ) ) {
 316+ wfSuppressWarnings();
 317+ $ok = file_put_contents( "{$dirRoot}/.htaccess", "Deny from all\n" );
 318+ wfRestoreWarnings();
 319+ if ( !$ok ) {
 320+ $storeDir = "mwstore://{$this->name}/{$container}";
 321+ $status->fatal( 'backend-fail-create', "$storeDir/.htaccess" );
 322+ return $status;
 323+ }
322324 }
323325 }
324326 return $status;
325327 }
326328
327329 /**
328 - * @see FileBackend::doClean()
 330+ * @see FileBackend::doCleanInternal()
329331 */
330 - protected function doClean( $container, $dir, array $params ) {
 332+ protected function doCleanInternal( $container, $dir, array $params ) {
331333 $status = Status::newGood();
332334 wfSuppressWarnings();
333335 if ( is_dir( $dir ) ) {
Index: trunk/phase3/includes/filerepo/backend/FileBackend.php
——@@ -292,9 +292,19 @@
293293 * @param $params Array
294294 * @return Status
295295 */
296 - abstract public function prepare( array $params );
 296+ final public function prepare( array $params ) {
 297+ if ( $this->readOnly != '' ) {
 298+ return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly );
 299+ }
 300+ return $this->doPrepare( $params );
 301+ }
297302
298303 /**
 304+ * @see FileBackendBase::prepare()
 305+ */
 306+ abstract protected function doPrepare( array $params );
 307+
 308+ /**
299309 * Take measures to block web access to a directory and
300310 * the container it belongs to. FS backends might add .htaccess
301311 * files wheras backends like Swift this might restrict container
——@@ -309,9 +319,19 @@
310320 * @param $params Array
311321 * @return Status
312322 */
313 - abstract public function secure( array $params );
 323+ final public function secure( array $params ) {
 324+ if ( $this->readOnly != '' ) {
 325+ return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly );
 326+ }
 327+ return $this->doSecure( $params );
 328+ }
314329
315330 /**
 331+ * @see FileBackendBase::secure()
 332+ */
 333+ abstract protected function doSecure( array $params );
 334+
 335+ /**
316336 * Clean up an empty storage directory.
317337 * On FS backends, the directory will be deleted. Others may do nothing.
318338 *
——@@ -321,9 +341,19 @@
322342 * @param $params Array
323343 * @return Status
324344 */
325 - abstract public function clean( array $params );
 345+ final public function clean( array $params ) {
 346+ if ( $this->readOnly != '' ) {
 347+ return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly );
 348+ }
 349+ return $this->doClean( $params );
 350+ }
326351
327352 /**
 353+ * @see FileBackendBase::clean()
 354+ */
 355+ abstract protected function doClean( array $params );
 356+
 357+ /**
328358 * Check if a file exists at a storage path in the backend.
329359 * This returns false if only a directory exists at the path.
330360 *
——@@ -747,9 +777,9 @@
748778 }
749779
750780 /**
751 - * @see FileBackendBase::prepare()
 781+ * @see FileBackendBase::doPrepare()
752782 */
753 - final public function prepare( array $params ) {
 783+ final protected function doPrepare( array $params ) {
754784 $status = Status::newGood();
755785 list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] );
756786 if ( $dir === null ) {
——@@ -757,28 +787,28 @@
758788 return $status; // invalid storage path
759789 }
760790 if ( $shard !== null ) { // confined to a single container/shard
761 - $status->merge( $this->doPrepare( $fullCont, $dir, $params ) );
 791+ $status->merge( $this->doPrepareInternal( $fullCont, $dir, $params ) );
762792 } else { // directory is on several shards
763793 wfDebug( __METHOD__ . ": iterating over all container shards.\n" );
764794 list( $b, $shortCont, $r ) = self::splitStoragePath( $params['dir'] );
765795 foreach ( $this->getContainerSuffixes( $shortCont ) as $suffix ) {
766 - $status->merge( $this->doPrepare( "{$fullCont}{$suffix}", $dir, $params ) );
 796+ $status->merge( $this->doPrepareInternal( "{$fullCont}{$suffix}", $dir, $params ) );
767797 }
768798 }
769799 return $status;
770800 }
771801
772802 /**
773 - * @see FileBackend::prepare()
 803+ * @see FileBackend::doPrepare()
774804 */
775 - protected function doPrepare( $container, $dir, array $params ) {
 805+ protected function doPrepareInternal( $container, $dir, array $params ) {
776806 return Status::newGood();
777807 }
778808
779809 /**
780 - * @see FileBackendBase::secure()
 810+ * @see FileBackendBase::doSecure()
781811 */
782 - final public function secure( array $params ) {
 812+ final protected function doSecure( array $params ) {
783813 $status = Status::newGood();
784814 list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] );
785815 if ( $dir === null ) {
——@@ -786,28 +816,28 @@
787817 return $status; // invalid storage path
788818 }
789819 if ( $shard !== null ) { // confined to a single container/shard
790 - $status->merge( $this->doSecure( $fullCont, $dir, $params ) );
 820+ $status->merge( $this->doSecureInternal( $fullCont, $dir, $params ) );
791821 } else { // directory is on several shards
792822 wfDebug( __METHOD__ . ": iterating over all container shards.\n" );
793823 list( $b, $shortCont, $r ) = self::splitStoragePath( $params['dir'] );
794824 foreach ( $this->getContainerSuffixes( $shortCont ) as $suffix ) {
795 - $status->merge( $this->doSecure( "{$fullCont}{$suffix}", $dir, $params ) );
 825+ $status->merge( $this->doSecureInternal( "{$fullCont}{$suffix}", $dir, $params ) );
796826 }
797827 }
798828 return $status;
799829 }
800830
801831 /**
802 - * @see FileBackend::secure()
 832+ * @see FileBackend::doSecure()
803833 */
804 - protected function doSecure( $container, $dir, array $params ) {
 834+ protected function doSecureInternal( $container, $dir, array $params ) {
805835 return Status::newGood();
806836 }
807837
808838 /**
809 - * @see FileBackendBase::clean()
 839+ * @see FileBackendBase::doClean()
810840 */
811 - final public function clean( array $params ) {
 841+ final protected function doClean( array $params ) {
812842 $status = Status::newGood();
813843 list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] );
814844 if ( $dir === null ) {
——@@ -815,21 +845,21 @@
816846 return $status; // invalid storage path
817847 }
818848 if ( $shard !== null ) { // confined to a single container/shard
819 - $status->merge( $this->doClean( $fullCont, $dir, $params ) );
 849+ $status->merge( $this->doCleanInternal( $fullCont, $dir, $params ) );
820850 } else { // directory is on several shards
821851 wfDebug( __METHOD__ . ": iterating over all container shards.\n" );
822852 list( $b, $shortCont, $r ) = self::splitStoragePath( $params['dir'] );
823853 foreach ( $this->getContainerSuffixes( $shortCont ) as $suffix ) {
824 - $status->merge( $this->doClean( "{$fullCont}{$suffix}", $dir, $params ) );
 854+ $status->merge( $this->doCleanInternal( "{$fullCont}{$suffix}", $dir, $params ) );
825855 }
826856 }
827857 return $status;
828858 }
829859
830860 /**
831 - * @see FileBackend::clean()
 861+ * @see FileBackend::doClean()
832862 */
833 - protected function doClean( $container, $dir, array $params ) {
 863+ protected function doCleanInternal( $container, $dir, array $params ) {
834864 return Status::newGood();
835865 }
836866

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r107986* Added FileBackendBase::getFileContents() function with a default FileBacken...aaron02:15, 4 January 2012

Status & tagging log