r108766 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r108765‎ | r108766 | r108767 >
Date:22:01, 12 January 2012
Author:aaron
Status:ok
Tags:filebackend 
Comment:
* Collapse multiple directory separators in FileBackend::normalizeStoragePath(). Also renamed the function to normalizeContainerPath() to be less confusing.
* Made SwiftFileBackend respect the 'latest' parameter of various functions via "X-Newest: true" header. Added to TODO in the doGetFileStat() function, as cloudfiles needs upstream changes for that.
* Fixed SwiftFileIterator to return paths relative to the given directory.
* Clean up for FSFileBackend trailing slash handling.
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/SwiftFileBackend.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/backend/FSFileBackend.php
——@@ -514,7 +514,8 @@
515515 * @param $dir string
516516 */
517517 public function __construct( $dir ) {
518 - $this->suffixStart = strlen( realpath( $dir ) ) + 1; // size of "path/to/dir/"
 518+ $dir = realpath( $dir ); // normalize
 519+ $this->suffixStart = strlen( $dir ) + 1; // size of "path/to/dir/"
519520 try {
520521 $flags = FilesystemIterator::CURRENT_AS_FILEINFO | FilesystemIterator::SKIP_DOTS;
521522 $this->iter = new RecursiveIteratorIterator(
Index: trunk/phase3/includes/filerepo/backend/SwiftFileBackend.php
——@@ -15,8 +15,6 @@
1616 * which is available at https://github.com/rackspace/php-cloudfiles.
1717 * All of the library classes must be registed in $wgAutoloadClasses.
1818 *
19 - * @TODO: handle 'latest' param as "X-Newest: true".
20 - *
2119 * @ingroup FileBackend
2220 */
2321 class SwiftFileBackend extends FileBackend {
——@@ -378,7 +376,7 @@
379377 protected function doSecureInternal( $fullCont, $dir, array $params ) {
380378 $status = Status::newGood();
381379 // @TODO: restrict container from $this->swiftProxyUser
382 - return $status; // badgers? We don't need no steenking badgers!
 380+ return $status;
383381 }
384382
385383 /**
——@@ -398,6 +396,7 @@
399397 $stat = false;
400398 try {
401399 $container = $conn->get_container( $srcCont );
 400+ // @TODO: handle 'latest' param as "X-Newest: true"
402401 $obj = $container->get_object( $srcRel );
403402 // Convert "Tue, 03 Jan 2012 22:01:04 GMT" to TS_MW
404403 $date = DateTime::createFromFormat( 'D, d F Y G:i:s e', $obj->last_modified );
——@@ -440,7 +439,7 @@
441440 try {
442441 $container = $conn->get_container( $srcCont );
443442 $obj = $container->get_object( $srcRel );
444 - $data = $obj->read();
 443+ $data = $obj->read( $this->headersFromParams( $params ) );
445444 } catch ( NoSuchContainerException $e ) {
446445 } catch ( NoSuchObjectException $e ) {
447446 } catch ( InvalidResponseException $e ) {
——@@ -462,7 +461,7 @@
463462 * Do not call this function outside of SwiftFileIterator
464463 *
465464 * @param $fullCont string Resolved container name
466 - * @param $dir string Resolved storage directory
 465+ * @param $dir string Resolved storage directory with no trailing slash
467466 * @param $after string Storage path of file to list items after
468467 * @param $limit integer Max number of items to list
469468 * @return Array
——@@ -476,7 +475,7 @@
477476 $files = array();
478477 try {
479478 $container = $conn->get_container( $fullCont );
480 - $files = $container->list_objects( $limit, $after, $dir );
 479+ $files = $container->list_objects( $limit, $after, "{$dir}/" );
481480 } catch ( NoSuchContainerException $e ) {
482481 } catch ( NoSuchObjectException $e ) {
483482 } catch ( InvalidResponseException $e ) {
——@@ -534,8 +533,8 @@
535534 }
536535
537536 try {
538 - $output = fopen("php://output", "w");
539 - $obj->stream( $output );
 537+ $output = fopen( "php://output", "w" );
 538+ $obj->stream( $output, $this->headersFromParams( $params ) );
540539 } catch ( InvalidResponseException $e ) {
541540 $status->fatal( 'backend-fail-connect', $this->name );
542541 } catch ( Exception $e ) { // some other exception?
——@@ -571,13 +570,17 @@
572571 try {
573572 $cont = $conn->get_container( $srcCont );
574573 $obj = $cont->get_object( $srcRel );
575 - $obj->save_to_filename( $tmpFile->getPath() );
 574+ $handle = fopen( $tmpFile->getPath(), 'w' );
 575+ if ( $handle ) {
 576+ $obj->stream( $handle, $this->headersFromParams( $params ) );
 577+ fclose( $handle );
 578+ } else {
 579+ $tmpFile = null; // couldn't open temp file
 580+ }
576581 } catch ( NoSuchContainerException $e ) {
577582 $tmpFile = null;
578583 } catch ( NoSuchObjectException $e ) {
579584 $tmpFile = null;
580 - } catch ( IOException $e ) {
581 - $tmpFile = null;
582585 } catch ( InvalidResponseException $e ) {
583586 $tmpFile = null;
584587 } catch ( Exception $e ) { // some other exception?
——@@ -589,6 +592,22 @@
590593 }
591594
592595 /**
 596+ * Get headers to send to Swift when reading a file based
 597+ * on a FileBackend params array, e.g. that of getLocalCopy().
 598+ * $params is currently only checked for a 'latest' flag.
 599+ *
 600+ * @param $params Array
 601+ * @return Array
 602+ */
 603+ protected function headersFromParams( array $params ) {
 604+ $hdrs = array();
 605+ if ( !empty( $params['latest'] ) ) {
 606+ $hdrs[] = 'X-Newest: true';
 607+ }
 608+ return $hdrs;
 609+ }
 610+
 611+ /**
593612 * Get a connection to the swift proxy
594613 *
595614 * @return CF_Connection|false
——@@ -644,6 +663,7 @@
645664 protected $backend;
646665 protected $container; //
647666 protected $dir; // string storage directory
 667+ protected $suffixStart; // integer
648668
649669 const PAGE_SIZE = 5000; // file listing buffer size
650670
——@@ -652,16 +672,20 @@
653673 *
654674 * @param $backend SwiftFileBackend
655675 * @param $fullCont string Resolved container name
656 - * @param $dir string Resolved relateive path
 676+ * @param $dir string Resolved relative directory
657677 */
658678 public function __construct( SwiftFileBackend $backend, $fullCont, $dir ) {
 679+ $this->backend = $backend;
659680 $this->container = $fullCont;
660681 $this->dir = $dir;
661 - $this->backend = $backend;
 682+ if ( substr( $this->dir, -1 ) === '/' ) {
 683+ $this->dir = substr( $this->dir, 0, -1 ); // remove trailing slash
 684+ }
 685+ $this->suffixStart = strlen( $dir ) + 1; // size of "path/to/dir/"
662686 }
663687
664688 public function current() {
665 - return current( $this->bufferIter );
 689+ return substr( current( $this->bufferIter ), $this->suffixStart );
666690 }
667691
668692 public function key() {
Index: trunk/phase3/includes/filerepo/backend/FileBackend.php
——@@ -1226,12 +1226,14 @@
12271227 * Null is returned if the path involves directory traversal.
12281228 * Traversal is insecure for FS backends and broken for others.
12291229 *
1230 - * @param $path string
 1230+ * @param $path string Storage path relative to a container
12311231 * @return string|null
12321232 */
1233 - final protected static function normalizeStoragePath( $path ) {
 1233+ final protected static function normalizeContainerPath( $path ) {
12341234 // Normalize directory separators
12351235 $path = strtr( $path, '\\', '/' );
 1236+ // Collapse consecutive directory separators
 1237+ $path = preg_replace( '![/]{2,}!', '/', $path );
12361238 // Use the same traversal protection as Title::secureAndSplit()
12371239 if ( strpos( $path, '.' ) !== false ) {
12381240 if (
——@@ -1264,7 +1266,7 @@
12651267 final protected function resolveStoragePath( $storagePath ) {
12661268 list( $backend, $container, $relPath ) = self::splitStoragePath( $storagePath );
12671269 if ( $backend === $this->name ) { // must be for this backend
1268 - $relPath = self::normalizeStoragePath( $relPath );
 1270+ $relPath = self::normalizeContainerPath( $relPath );
12691271 if ( $relPath !== null ) {
12701272 // Get shard for the normalized path if this container is sharded
12711273 $cShard = $this->getContainerShard( $container, $relPath );

Status & tagging log