r104706 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r104705‎ | r104706 | r104707 >
Date:17:27, 30 November 2011
Author:aaron
Status:deferred
Tags:
Comment:
* Made FileRepo::publish() support FS source paths again
* Updated UnregisteredLocalFile
* Removed bogus field access in simpleClean()
* Made splitStoragePath() accept container paths
* Fixed FSFileBackend::move()/delete() in terms of when we delete the source path (avoids collisions in op batches)
* Fixes and cleanups after upload testing
Modified paths:
  • /branches/FileBackend/phase3/includes/GlobalFunctions.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/FileRepo.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/FSFileBackend.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/FileBackendMultiWrite.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/FileOp.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/LockManager.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/file/FSFile.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/file/File.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/file/LocalFile.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/file/TempFSFile.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/file/UnregisteredLocalFile.php (modified) (history)
  • /branches/FileBackend/phase3/includes/media/Generic.php (modified) (history)
  • /branches/FileBackend/phase3/includes/upload/UploadStash.php (modified) (history)

Diff [purge]

Index: branches/FileBackend/phase3/includes/upload/UploadStash.php
——@@ -233,7 +233,7 @@
234234 'us_user' => $this->userId,
235235 'us_key' => $key,
236236 'us_orig_path' => $path,
237 - 'us_path' => $stashPath,
 237+ 'us_path' => $stashPath, // virtual URL
238238 'us_size' => $fileProps['size'],
239239 'us_sha1' => $fileProps['sha1'],
240240 'us_mime' => $fileProps['mime'],
Index: branches/FileBackend/phase3/includes/GlobalFunctions.php
——@@ -2387,6 +2387,11 @@
23882388 wfDebug( "$caller: called wfMkdirParents($dir)\n" );
23892389 }
23902390
 2391+ if ( FileBackend::isStoragePath( $dir ) ) {
 2392+ throw new MWException( "Given virtual path `$dir`." );
 2393+ return false;
 2394+ }
 2395+
23912396 if( strval( $dir ) === '' || file_exists( $dir ) ) {
23922397 return true;
23932398 }
Index: branches/FileBackend/phase3/includes/filerepo/file/LocalFile.php
——@@ -881,7 +881,7 @@
882882
883883 /**
884884 * Upload a file and record it in the DB
885 - * @param $srcPath String: source path or virtual URL
 885+ * @param $srcPath String: source storage path or virtual URL
886886 * @param $comment String: upload description
887887 * @param $pageText String: text to use for the new description page,
888888 * if a new description page is created
——@@ -1003,6 +1003,7 @@
10041004 if ( $dbw->affectedRows() == 0 ) {
10051005 $reupload = true;
10061006
 1007+ #if ( !$oldver ) wfDebugDieBacktrace();
10071008 # Collision, this is an update of a file
10081009 # Insert previous contents into oldimage
10091010 $dbw->insertSelect( 'oldimage', 'image',
Index: branches/FileBackend/phase3/includes/filerepo/file/UnregisteredLocalFile.php
——@@ -27,8 +27,8 @@
2828 var $handler;
2929
3030 /**
31 - * @param $path
32 - * @param $mime
 31+ * @param $path string Storage path
 32+ * @param $mime string
3333 * @return UnregisteredLocalFile
3434 */
3535 static function newFromPath( $path, $mime ) {
——@@ -135,10 +135,11 @@
136136 }
137137
138138 function getSize() {
139 - if ( file_exists( $this->path ) ) {
140 - return filesize( $this->path );
141 - } else {
142 - return false;
 139+ $this->assertRepoDefined();
 140+ $props = $this->repo->getFileProps( $this->path );
 141+ if ( isset( $props['size'] ) ) {
 142+ return $props['size'];
143143 }
 144+ return false; // doesn't exist
144145 }
145146 }
Index: branches/FileBackend/phase3/includes/filerepo/file/TempFSFile.php
——@@ -15,6 +15,9 @@
1616 class TempFSFile extends FSFile {
1717 protected $canDelete = true; // garbage collect the temp file
1818
 19+ /** @var Array of active temp files to purge on shutdown */
 20+ protected static $instances = array();
 21+
1922 /**
2023 * Make a new temporary file on the file system
2124 *
——@@ -35,7 +38,10 @@
3639 } else {
3740 $path = $tmpDest;
3841 }
39 - return new self( $path );
 42+ $tmpFile = new self( $path );
 43+ self::$instances[] = $tmpFile;
 44+
 45+ return $tmpFile;
4046 }
4147
4248 /**
——@@ -48,8 +54,8 @@
4955 }
5056
5157 /**
52 - * Cleans up after the temporary file.
53 - * Currently this means removing it from the local disk.
 58+ * Cleans up after the temporary file by deleting it.
 59+ * This is done on shutdown after PHP kills self::$instances.
5460 */
5561 function __destruct() {
5662 if ( $this->canDelete ) {
Index: branches/FileBackend/phase3/includes/filerepo/file/File.php
——@@ -118,6 +118,7 @@
119119 /**
120120 * Given a string or Title object return either a
121121 * valid Title object with namespace NS_FILE or null
 122+ *
122123 * @param $title Title|string
123124 * @param $exception string|false Use 'exception' to throw an error on bad titles
124125 * @return Title|null
——@@ -243,6 +244,7 @@
244245
245246 /**
246247 * Return the associated title object
 248+ *
247249 * @return Title|false
248250 */
249251 public function getTitle() {
——@@ -850,6 +852,7 @@
851853
852854 /**
853855 * Get a MediaHandler instance for this file
 856+ *
854857 * @return MediaHandler
855858 */
856859 function getHandler() {
——@@ -861,6 +864,7 @@
862865
863866 /**
864867 * Get a ThumbnailImage representing a file type icon
 868+ *
865869 * @return ThumbnailImage
866870 */
867871 function iconThumb() {
Index: branches/FileBackend/phase3/includes/filerepo/file/FSFile.php
——@@ -113,8 +113,14 @@
114114 return $info;
115115 }
116116
 117+ /**
 118+ * Placeholder file properties to use for files that don't exist
 119+ *
 120+ * @return Array
 121+ */
117122 public static function placeholderProps() {
118123 $info = array();
 124+ $info['fileExists'] = false;
119125 $info['mime'] = null;
120126 $info['media_type'] = MEDIATYPE_UNKNOWN;
121127 $info['metadata'] = '';
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackendMultiWrite.php
——@@ -6,15 +6,15 @@
77 */
88
99 /**
10 - * This class defines a multi-write backend. Multiple backends can
11 - * be registered to this proxy backend it will act as a single backend.
12 - * Use this when all access to the backends is through this proxy backend.
 10+ * This class defines a multi-write backend. Multiple backends can be
 11+ * registered to this proxy backend and it will act as a single backend.
 12+ * Use this when all access to those backends is through this proxy backend.
1313 * At least one of the backends must be declared the "master" backend.
1414 *
1515 * The order that the backends are defined sets the priority of which
1616 * backend is read from or written to first. Functions like fileExists()
1717 * and getFileProps() will return information based on the first backend
18 - * that has the file (normally both should have it anyway). Special cases:
 18+ * that has the file. Special cases are listed below:
1919 * a) getFileList() will return results from the first backend that is
2020 * not declared as non-persistent cache. This is for correctness.
2121 * b) getFileTimestamp() will always check only the master backend to
——@@ -25,6 +25,8 @@
2626 * All write operations are performed on all backends.
2727 * If an operation fails on one backend it will be rolled back from the others.
2828 *
 29+ * Non-persistent backends used for caching must be declared.
 30+ *
2931 * @ingroup FileRepo
3032 * @ingroup FileBackend
3133 */
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileOp.php
——@@ -16,6 +16,8 @@
1717 protected $params;
1818 /** $var FileBackendBase */
1919 protected $backend;
 20+ /** @var TempLocalFile|null */
 21+ protected $tmpSourceFile, $tmpDestFile;
2022
2123 protected $state;
2224 protected $failed;
——@@ -145,6 +147,31 @@
146148 * @return Status
147149 */
148150 abstract protected function doFinish();
 151+
 152+ /**
 153+ * Backup any file at the source to a temporary file
 154+ *
 155+ * @return Status
 156+ */
 157+ protected function backupSource() {
 158+ $status = Status::newGood();
 159+ // Check if a file already exists at the source...
 160+ $params = array( 'source' => $this->params['source'] );
 161+ if ( $this->backend->fileExists( $params ) ) {
 162+ // Create a temporary backup copy...
 163+ $this->tmpSourcePath = $this->backend->getLocalCopy( $params );
 164+ if ( $this->tmpSourcePath === null ) {
 165+ $status->fatal( 'backend-fail-backup', $this->params['source'] );
 166+ return $status;
 167+ }
 168+ } else {
 169+ if ( empty( $this->params['ignoreMissingSource'] ) ) {
 170+ $status->fatal( 'backend-fail-notexists', $this->params['source'] );
 171+ return $status;
 172+ }
 173+ }
 174+ return $status;
 175+ }
149176
150177 /**
151178 * Backup any file at the destination to a temporary file.
——@@ -164,7 +191,7 @@
165192
166193 if ( !empty( $this->params['overwriteDest'] ) ) {
167194 // Create a temporary backup copy...
168 - $this->tmpDestFile = $this->getLocalCopy( $this->params['dest'] );
 195+ $this->tmpDestFile = $this->backend->getLocalCopy( $params );
169196 if ( !$this->tmpDestFile ) {
170197 $status->fatal( 'backend-fail-backup', $this->params['dest'] );
171198 return $status;
——@@ -217,7 +244,7 @@
218245 if ( $this->backend->getHashType() === 'md5' ) {
219246 $hash = $this->backend->getFileHash( $path );
220247 } else {
221 - $tmp = $this->getLocalCopy( $path );
 248+ $tmp = $this->backend->getLocalCopy( array( 'source' => $path ) );
222249 if ( !$tmp ) {
223250 return false; // error
224251 }
——@@ -231,6 +258,27 @@
232259 }
233260
234261 /**
 262+ * Restore any temporary source backup file
 263+ *
 264+ * @return Status
 265+ */
 266+ protected function restoreSource() {
 267+ $status = Status::newGood();
 268+ // Restore any file that was at the destination
 269+ if ( $this->tmpSourcePath !== null ) {
 270+ $params = array(
 271+ 'source' => $this->tmpSourcePath,
 272+ 'dest' => $this->params['source']
 273+ );
 274+ $status = $this->backend->store( $params );
 275+ if ( !$status->isOK() ) {
 276+ return $status;
 277+ }
 278+ }
 279+ return $status;
 280+ }
 281+
 282+ /**
235283 * Restore any temporary destination backup file
236284 *
237285 * @return Status
——@@ -261,9 +309,6 @@
262310 * overwriteSame : override any existing file at destination
263311 */
264312 class StoreFileOp extends FileOp {
265 - /** @var TempLocalFile|null */
266 - protected $tmpDestFile; // temp copy of existing destination file
267 -
268313 protected function doPrecheck() {
269314 $status = Status::newGood();
270315 // Check if the source files exists on disk (FS)
——@@ -278,8 +323,7 @@
279324
280325 protected function doAttempt() {
281326 // Store the file at the destination
282 - $status = $this->backend->store( $this->params );
283 - return $status;
 327+ return $this->backend->store( $this->params );
284328 }
285329
286330 protected function doRevert() {
——@@ -290,7 +334,7 @@
291335 return $status; // also can't restore any dest file
292336 }
293337 // Restore any file that was at the destination
294 - $status = $this->restoreDest();
 338+ $status->merge( $this->restoreDest() );
295339 return $status;
296340 }
297341
——@@ -318,14 +362,12 @@
319363 class CreateFileOp extends FileOp {
320364 protected function doPrecheck() {
321365 // Create a destination backup copy as needed
322 - $status = $this->checkAndBackupDest();
323 - return $status;
 366+ return $this->checkAndBackupDest();
324367 }
325368
326369 protected function doAttempt() {
327370 // Create the file at the destination
328 - $status = $this->backend->create( $this->params );
329 - return $status;
 371+ return $this->backend->create( $this->params );
330372 }
331373
332374 protected function doRevert() {
——@@ -336,7 +378,7 @@
337379 return $status; // also can't restore any dest file
338380 }
339381 // Restore any file that was at the destination
340 - $status = $this->restoreDest();
 382+ $status->merge( $this->restoreDest() );
341383 return $status;
342384 }
343385
——@@ -377,8 +419,7 @@
378420
379421 protected function doAttempt() {
380422 // Copy the file into the destination
381 - $status = $this->backend->copy( $this->params );
382 - return $status;
 423+ return $this->backend->copy( $this->params );
383424 }
384425
385426 protected function doRevert() {
——@@ -389,7 +430,7 @@
390431 return $status; // also can't restore any dest file
391432 }
392433 // Restore any file that was at the destination
393 - $status = $this->restoreDest();
 434+ $status->merge( $this->restoreDest() );
394435 return $status;
395436 }
396437
——@@ -432,6 +473,9 @@
433474 }
434475 // Create a destination backup copy as needed
435476 $status->merge( $this->checkAndBackupDest() );
 477+ if ( !$status->isOK() ) {
 478+ return $status;
 479+ }
436480 return $status;
437481 }
438482
——@@ -439,9 +483,19 @@
440484 // Native moves: move the file into the destination
441485 if ( $this->usingMove ) {
442486 $status = $this->backend->move( $this->params );
443 - // Non-native moves: copy the file into the destination
 487+ // Non-native moves: copy the file into the destination & delete source
444488 } else {
 489+ // Copy source to dest
445490 $status = $this->backend->copy( $this->params );
 491+ if ( !$status->isOK() ) {
 492+ return $status;
 493+ }
 494+ // Delete source
 495+ $params = array( 'source' => $this->params['source'] );
 496+ $status = $this->backend->delete( $params );
 497+ if ( !$status->isOK() ) {
 498+ return $status;
 499+ }
446500 }
447501 return $status;
448502 }
——@@ -459,6 +513,13 @@
460514 }
461515 // Non-native moves: remove the file saved to the destination
462516 } else {
 517+ // Copy destination back to source
 518+ $params = array( 'source' => $this->params['dest'], 'dest' => $this->params['source'] );
 519+ $status = $this->backend->copy( $params );
 520+ if ( !$status->isOK() ) {
 521+ return $status; // also can't restore any dest file
 522+ }
 523+ // Delete destination
463524 $params = array( 'source' => $this->params['dest'] );
464525 $status = $this->backend->delete( $params );
465526 if ( !$status->isOK() ) {
——@@ -466,20 +527,12 @@
467528 }
468529 }
469530 // Restore any file that was at the destination
470 - $status = $this->restoreDest();
 531+ $status->merge( $this->restoreDest() );
471532 return $status;
472533 }
473534
474535 protected function doFinish() {
475 - // Native moves: nothing is at the source anymore
476 - if ( $this->usingMove ) {
477 - $status = Status::newGood();
478 - // Non-native moves: delete the source file
479 - } else {
480 - $params = array( 'source' => $this->params['source'] );
481 - $status = $this->backend->delete( $params );
482 - }
483 - return $status;
 536+ return Status::newGood();
484537 }
485538
486539 protected function getSourceMD5() {
——@@ -502,14 +555,12 @@
503556 class ConcatenateFileOp extends FileOp {
504557 protected function doPrecheck() {
505558 // Create a destination backup copy as needed
506 - $status = $this->checkAndBackupDest();
507 - return $status;
 559+ return $this->checkAndBackupDest();
508560 }
509561
510562 protected function doAttempt() {
511563 // Concatenate the file at the destination
512 - $status = $this->backend->concatenate( $this->params );
513 - return $status;
 564+ return $this->backend->concatenate( $this->params );
514565 }
515566
516567 protected function doRevert() {
——@@ -520,7 +571,7 @@
521572 return $status; // also can't restore any dest file
522573 }
523574 // Restore any file that was at the destination
524 - $status = $this->restoreDest();
 575+ $status->merge( $this->restoreDest() );
525576 return $status;
526577 }
527578
——@@ -546,28 +597,28 @@
547598 class DeleteFileOp extends FileOp {
548599 protected function doPrecheck() {
549600 $status = Status::newGood();
550 - if ( empty( $this->params['ignoreMissingSource'] ) ) {
551 - $params = array( 'source' => $this->params['source'] );
552 - if ( !$this->backend->fileExists( $params ) ) {
553 - $status->fatal( 'backend-fail-notexists', $this->params['source'] );
554 - return $status;
555 - }
 601+ $params = array( 'source' => $this->params['source'] );
 602+ if ( $this->backend->fileExists( $params ) ) {
 603+ // Create a source backup copy as needed
 604+ $status->merge( $this->backupSource() );
 605+ } elseif ( empty( $this->params['ignoreMissingSource'] ) ) {
 606+ $status->fatal( 'backend-fail-notexists', $this->params['source'] );
556607 }
557608 return $status;
558609 }
559610
560611 protected function doAttempt() {
561 - return Status::newGood();
 612+ // Delete the source file
 613+ return $this->backend->delete( $this->params );
562614 }
563615
564616 protected function doRevert() {
565 - return Status::newGood();
 617+ // Restore any source file
 618+ return $this->restoreSource();
566619 }
567620
568621 protected function doFinish() {
569 - // Delete the source file
570 - $status = $this->backend->delete( $this->params );
571 - return $status;
 622+ return Status::newGood();
572623 }
573624
574625 function storagePathsUsed() {
Index: branches/FileBackend/phase3/includes/filerepo/backend/FSFileBackend.php
——@@ -76,6 +76,15 @@
7777 }
7878
7979 function copy( array $params ) {
 80+ $status = Status::newGood();
 81+
 82+ list( $c, $source ) = $this->resolveStoragePath( $params['source'] );
 83+ if ( $source === null ) {
 84+ $status->fatal( 'backend-fail-invalidpath', $params['source'] );
 85+ return $status;
 86+ }
 87+ $params['source'] = $source; // resolve source to FS path
 88+
8089 return $this->store( $params ); // both source and dest are on FS
8190 }
8291
Index: branches/FileBackend/phase3/includes/filerepo/backend/LockManager.php
——@@ -287,7 +287,7 @@
288288 *
289289 * @param Array $config
290290 */
291 - function __construct( array $config ) {
 291+ public function __construct( array $config ) {
292292 // Sanitize dbsByBucket config to prevent PHP errors
293293 $this->dbsByBucket = array_filter( $config['dbsByBucket'], 'is_array' );
294294 $this->dbsByBucket = array_values( $this->dbsByBucket ); // consecutive
——@@ -671,8 +671,6 @@
672672 * Simple version of LockManager that does nothing
673673 */
674674 class NullLockManager extends LockManager {
675 - function __construct( array $config ) {}
676 -
677675 protected function doLock( array $keys, $type ) {
678676 return Status::newGood();
679677 }
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php
——@@ -521,7 +521,9 @@
522522 // Note: strlen( 'mwstore://' ) = 10
523523 $parts = explode( '/', substr( $storagePath, 10 ), 3 );
524524 if ( count( $parts ) == 3 ) {
525 - return $parts;
 525+ return $parts; // e.g. "backend/container/path"
 526+ } elseif ( count( $parts ) == 2 ) {
 527+ return array( $parts[0], $parts[1], '' ); // e.g. "backend/container"
526528 }
527529 }
528530 return array( null, null, null );
Index: branches/FileBackend/phase3/includes/filerepo/FileRepo.php
——@@ -786,8 +786,8 @@
787787 }
788788
789789 /**
790 - * Copy or move a file either from a storage path or from a mwrepo://
791 - * virtual URL, into this repository at the specified destination location.
 790+ * Copy or move a file either from a storage path, virtual URL,
 791+ * or FS path, into this repository at the specified destination location.
792792 *
793793 * Returns a FileRepoStatus object. On success, the value contains "new" or
794794 * "archived", to indicate whether the file was new with that name.
——@@ -883,20 +883,33 @@
884884 } else {
885885 $status->value[$i] = 'new';
886886 }
887 - if ( $flags & self::DELETE_SOURCE ) {
 887+ // Copy (or move) the source file to the destination
 888+ if ( FileBackend::isStoragePath( $srcPath ) ) {
 889+ if ( $flags & self::DELETE_SOURCE ) {
 890+ $operations[] = array(
 891+ 'op' => 'move',
 892+ 'source' => $srcPath,
 893+ 'dest' => $dstPath,
 894+ 'ignoreErrors' => true
 895+ );
 896+ } else {
 897+ $operations[] = array(
 898+ 'op' => 'copy',
 899+ 'source' => $srcPath,
 900+ 'dest' => $dstPath,
 901+ 'ignoreErrors' => true
 902+ );
 903+ }
 904+ } else { // FS source path
888905 $operations[] = array(
889 - 'op' => 'move',
 906+ 'op' => 'store',
890907 'source' => $srcPath,
891908 'dest' => $dstPath,
892909 'ignoreErrors' => true
893910 );
894 - } else {
895 - $operations[] = array(
896 - 'op' => 'copy',
897 - 'source' => $srcPath,
898 - 'dest' => $dstPath,
899 - 'ignoreErrors' => true
900 - );
 911+ if ( $flags & self::DELETE_SOURCE ) {
 912+ $sourceFSFilesToDelete[] = $srcPath;
 913+ }
901914 }
902915 }
903916
——@@ -1229,14 +1242,9 @@
12301243 if ( !isset( $this->simpleCleanPairs ) ) {
12311244 global $IP;
12321245 $this->simpleCleanPairs = array(
1233 - $this->directory => 'public',
1234 - "{$this->directory}/temp" => 'temp',
12351246 $IP => '$IP',
12361247 dirname( __FILE__ ) => '$IP/extensions/WebStore', // WTF
12371248 );
1238 - if ( $this->deletedDir ) {
1239 - $this->simpleCleanPairs[$this->deletedDir] = 'deleted';
1240 - }
12411249 }
12421250 return strtr( $param, $this->simpleCleanPairs );
12431251 }
Index: branches/FileBackend/phase3/includes/media/Generic.php
——@@ -211,12 +211,12 @@
212212 return new MediaTransformError( 'thumbnail_error',
213213 $params['width'], 0, wfMsg( 'thumbnail-temp-create' ) );
214214 }
215 - $tmpDest = $tmpFile->getPath();
 215+ $tmpDest = $tmpFile->getPath(); // path of 0-byte temp file
216216 // Create the output thumbnail on the FS
217217 $out = $this->doFSTransform( $image, $tmpDest, $dstUrl, $params, $flags );
218218 // Copy any thumbnail from FS into storage at $dstpath
219219 // Note: no file is created if it's to be rendered client-side.
220 - if ( !$out->isError() && is_file( $tmpDest ) ) {
 220+ if ( !$out->isError() && filesize( $tmpDest ) ) {
221221 $op = array( 'op' => 'store',
222222 'source' => $tmpDest, 'dest' => $dstPath, 'overwriteDest' => true );
223223 if ( !$image->getRepo()->getBackend()->doOperation( $op )->isOK() ) {

Status & tagging log