r109980 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r109979‎ | r109980 | r109981 >
Date:01:57, 25 January 2012
Author:aaron
Status:ok (Comments)
Tags:filebackend 
Comment:
In FileBackend:
* Use 'b' param in some fopen() calls as needed for Windows and newline handling.
* Removed some useless padding code in FileBackend::getContainerShard(). Initialized $m to make IDE happy.
* Updated some code comments.
In SwiftFileBackend:
* Manually set the ETag when using php-cloudfiles for creating files to avoid https://github.com/rackspace/php-cloudfiles/issues/59.
* Manually set the content type based on how StreamFile::getType(). This makes it safe to read files directly out of the proxy to end-users. The streamFile() backend functions already uses a similar content-type check.
Modified paths:
  • /trunk/phase3/includes/StreamFile.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FSFile.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileBackend.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/FileOp.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/SwiftFileBackend.php (modified) (history)
  • /trunk/phase3/includes/filerepo/backend/TempFSFile.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php
——@@ -412,21 +412,22 @@
413413 /**
414414 * @dataProvider provider_testCreate
415415 */
416 - public function testCreate( $op, $dest, $alreadyExists, $okStatus, $newSize ) {
 416+ public function testCreate( $op, $alreadyExists, $okStatus, $newSize ) {
417417 $this->backend = $this->singleBackend;
418418 $this->tearDownFiles();
419 - $this->doTestCreate( $op, $dest, $alreadyExists, $okStatus, $newSize );
 419+ $this->doTestCreate( $op, $alreadyExists, $okStatus, $newSize );
420420 $this->tearDownFiles();
421421
422422 $this->backend = $this->multiBackend;
423423 $this->tearDownFiles();
424 - $this->doTestCreate( $op, $dest, $alreadyExists, $okStatus, $newSize );
 424+ $this->doTestCreate( $op, $alreadyExists, $okStatus, $newSize );
425425 $this->tearDownFiles();
426426 }
427427
428 - private function doTestCreate( $op, $dest, $alreadyExists, $okStatus, $newSize ) {
 428+ private function doTestCreate( $op, $alreadyExists, $okStatus, $newSize ) {
429429 $backendName = $this->backendClass();
430430
 431+ $dest = $op['dst'];
431432 $this->backend->prepare( array( 'dir' => dirname( $dest ) ) );
432433
433434 $oldText = 'blah...blah...waahwaah';
——@@ -477,44 +478,52 @@
478479 public function provider_testCreate() {
479480 $cases = array();
480481
481 - $source = $this->baseStorePath() . '/unittest-cont2/myspacefile.txt';
 482+ $dest = $this->baseStorePath() . '/unittest-cont2/myspacefile.txt';
482483
483 - $dummyText = 'hey hey';
484 - $op = array( 'op' => 'create', 'content' => $dummyText, 'dst' => $source );
 484+ $op = array( 'op' => 'create', 'content' => 'test test testing', 'dst' => $dest );
485485 $cases[] = array(
486486 $op, // operation
487 - $source, // source
488487 false, // no dest already exists
489488 true, // succeeds
490 - strlen( $dummyText )
 489+ strlen( $op['content'] )
491490 );
492491
 492+ $op2 = $op;
 493+ $op2['content'] = "\n";
493494 $cases[] = array(
494 - $op, // operation
495 - $source, // source
 495+ $op2, // operation
 496+ false, // no dest already exists
 497+ true, // succeeds
 498+ strlen( $op2['content'] )
 499+ );
 500+
 501+ $op2 = $op;
 502+ $op2['content'] = "fsf\n waf 3kt";
 503+ $cases[] = array(
 504+ $op2, // operation
496505 true, // dest already exists
497506 false, // fails
498 - strlen( $dummyText )
 507+ strlen( $op2['content'] )
499508 );
500509
501510 $op2 = $op;
 511+ $op2['content'] = "egm'g gkpe gpqg eqwgwqg";
502512 $op2['overwrite'] = true;
503513 $cases[] = array(
504514 $op2, // operation
505 - $source, // source
506515 true, // dest already exists
507516 true, // succeeds
508 - strlen( $dummyText )
 517+ strlen( $op2['content'] )
509518 );
510519
511520 $op2 = $op;
 521+ $op2['content'] = "39qjmg3-qg";
512522 $op2['overwriteSame'] = true;
513523 $cases[] = array(
514524 $op2, // operation
515 - $source, // source
516525 true, // dest already exists
517526 false, // succeeds
518 - strlen( $dummyText )
 527+ strlen( $op2['content'] )
519528 );
520529
521530 return $cases;
Index: trunk/phase3/includes/filerepo/backend/FileOp.php
——@@ -213,7 +213,7 @@
214214 }
215215
216216 /**
217 - * Get required operation parameters
 217+ * Get the file operation parameters
218218 *
219219 * @return Array (required params list, optional params list)
220220 */
Index: trunk/phase3/includes/filerepo/backend/TempFSFile.php
——@@ -1,7 +1,6 @@
22 <?php
33 /**
44 * @file
5 - * @ingroup FileRepo
65 * @ingroup FileBackend
76 */
87
——@@ -9,7 +8,6 @@
109 * This class is used to hold the location and do limited manipulation
1110 * of files stored temporarily (usually this will be $wgTmpDirectory)
1211 *
13 - * @ingroup FileRepo
1412 * @ingroup FileBackend
1513 */
1614 class TempFSFile extends FSFile {
Index: trunk/phase3/includes/filerepo/backend/SwiftFileBackend.php
——@@ -138,6 +138,12 @@
139139 $obj = new CF_Object( $dContObj, $dstRel, false, false ); // skip HEAD
140140 // Note: metadata keys stored as [Upper case char][[Lower case char]...]
141141 $obj->metadata = array( 'Sha1base36' => $sha1Hash );
 142+ // Manually set the ETag (https://github.com/rackspace/php-cloudfiles/issues/59).
 143+ // The MD5 here will be checked within Swift against its own MD5.
 144+ $obj->set_etag( md5( $params['content'] ) );
 145+ // Use the same content type as StreamFile for security
 146+ $obj->content_type = StreamFile::contentTypeFromPath( $params['dst'] );
 147+ // Actually write the object in Swift
142148 $obj->write( $params['content'] );
143149 } catch ( BadContentTypeException $e ) {
144150 $status->fatal( 'backend-fail-contenttype', $params['dst'] );
——@@ -201,6 +207,11 @@
202208 $obj = new CF_Object( $dContObj, $dstRel, false, false ); // skip HEAD
203209 // Note: metadata keys stored as [Upper case char][[Lower case char]...]
204210 $obj->metadata = array( 'Sha1base36' => $sha1Hash );
 211+ // The MD5 here will be checked within Swift against its own MD5.
 212+ $obj->set_etag( md5_file( $params['src'] ) );
 213+ // Use the same content type as StreamFile for security
 214+ $obj->content_type = StreamFile::contentTypeFromPath( $params['dst'] );
 215+ // Actually write the object in Swift
205216 $obj->load_from_filename( $params['src'], True ); // calls $obj->write()
206217 } catch ( BadContentTypeException $e ) {
207218 $status->fatal( 'backend-fail-contenttype', $params['dst'] );
——@@ -585,7 +596,7 @@
586597 // Create a new temporary file...
587598 $tmpFile = TempFSFile::factory( wfBaseName( $srcRel ) . '_', $ext );
588599 if ( $tmpFile ) {
589 - $handle = fopen( $tmpFile->getPath(), 'w' );
 600+ $handle = fopen( $tmpFile->getPath(), 'wb' );
590601 if ( $handle ) {
591602 $obj->stream( $handle, $this->headersFromParams( $params ) );
592603 fclose( $handle );
Index: trunk/phase3/includes/filerepo/backend/FileBackend.php
——@@ -792,7 +792,7 @@
793793 }
794794
795795 // Build up the temp file using the source chunks (in order)...
796 - $tmpHandle = fopen( $tmpPath, 'a' );
 796+ $tmpHandle = fopen( $tmpPath, 'ab' );
797797 if ( $tmpHandle === false ) {
798798 $status->fatal( 'backend-fail-opentemp', $tmpPath );
799799 return $status;
——@@ -1448,8 +1448,9 @@
14491449 // Allow certain directories to be above the hash dirs so as
14501450 // to work with FileRepo (e.g. "archive/a/ab" or "temp/a/ab").
14511451 // They must be 2+ chars to avoid any hash directory ambiguity.
 1452+ $m = array();
14521453 if ( preg_match( "!^(?:[^/]{2,}/)*$hashDirRegex(?:/|$)!", $relPath, $m ) ) {
1453 - return '.' . str_pad( $m['shard'], $hashLevels, '0', STR_PAD_LEFT );
 1454+ return '.' . $m['shard'];
14541455 }
14551456 return null; // failed to match
14561457 }
Index: trunk/phase3/includes/filerepo/backend/FSFile.php
——@@ -1,14 +1,12 @@
22 <?php
33 /**
44 * @file
5 - * @ingroup FileRepo
65 * @ingroup FileBackend
76 */
87
98 /**
109 * Class representing a non-directory file on the file system
1110 *
12 - * @ingroup FileRepo
1311 * @ingroup FileBackend
1412 */
1513 class FSFile {
Index: trunk/phase3/includes/StreamFile.php
——@@ -74,7 +74,7 @@
7575 // Cancel output buffering and gzipping if set
7676 wfResetOutputBuffers();
7777
78 - $type = self::getType( $path );
 78+ $type = self::contentTypeFromPath( $path );
7979 if ( $type && $type != 'unknown/unknown' ) {
8080 header( "Content-type: $type" );
8181 } else {
——@@ -115,12 +115,13 @@
116116 }
117117
118118 /**
119 - * Determine the filetype we're dealing with
 119+ * Determine the file type of a file based on the path
 120+ *
120121 * @param $filename string Storage path or file system path
121122 * @param $safe bool Whether to do retroactive upload blacklist checks
122123 * @return null|string
123124 */
124 - protected static function getType( $filename, $safe = true ) {
 125+ public static function contentTypeFromPath( $filename, $safe = true ) {
125126 global $wgTrivialMimeDetection;
126127
127128 $ext = strrchr( $filename, '.' );

Comments

#Comment by Tim Starling (talk | contribs)   03:44, 1 February 2012

I may have misled you a bit with my outdated knowledge of binary mode: "As of PHP 4.3.2, the default mode is set to binary for all platforms that distinguish between binary and text mode." [1]. Oh well, it's harmless.

Status & tagging log