r105767 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r105766‎ | r105767 | r105768 >
Date:18:22, 10 December 2011
Author:aaron
Status:deferred
Tags:
Comment:
* Fixed checkContainerName() as it should be alphaNUMERIC
* Moved default getLocalReference() implementation down to FileBackend
* Removed overkill sanity check from img_auth.php
* Made NewParserTest delete the table rows it makes
* Added NullLockManager to autoloader
* Fixed 'rmdir(...\Temp/mwParser-1353610175-images/thumb/3/3a/Foobar.jpg)' error in tests
* EXIF & metadata test fixes
* Added MTO::getLocalCopyPath() and let backend instances be passed in as repo config (useful for tests)
Modified paths:
  • /branches/FileBackend/phase3/img_auth.php (modified) (history)
  • /branches/FileBackend/phase3/includes/AutoLoader.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/FileRepo.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php (modified) (history)
  • /branches/FileBackend/phase3/includes/filerepo/backend/LockManager.php (modified) (history)
  • /branches/FileBackend/phase3/includes/media/Generic.php (modified) (history)
  • /branches/FileBackend/phase3/includes/media/MediaTransformOutput.php (modified) (history)
  • /branches/FileBackend/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php (modified) (history)
  • /branches/FileBackend/phase3/tests/phpunit/includes/media/ExifRotationTest.php (modified) (history)
  • /branches/FileBackend/phase3/tests/phpunit/includes/media/FormatMetadataTest.php (modified) (history)
  • /branches/FileBackend/phase3/tests/phpunit/includes/media/GIFTest.php (modified) (history)
  • /branches/FileBackend/phase3/tests/phpunit/includes/media/PNGTest.php (modified) (history)
  • /branches/FileBackend/phase3/tests/phpunit/includes/parser/NewParserTest.php (modified) (history)

Diff [purge]

Index: branches/FileBackend/phase3/img_auth.php
——@@ -74,9 +74,6 @@
7575
7676 // Get the local file repository
7777 $repo = RepoGroup::singleton()->getRepo( 'local' );
78 - if ( !$repo ) {
79 - return; // wtf?
80 - }
8178
8279 // Get the full file storage path and extract the source file name.
8380 // (e.g. 120px-Foo.png => Foo.png or page2-120px-Foo.png => Foo.png).
Index: branches/FileBackend/phase3/tests/phpunit/includes/parser/NewParserTest.php
——@@ -119,6 +119,7 @@
120120 }
121121
122122 function addDBData() {
 123+ $this->tablesUsed[] = 'image';
123124 # Hack: insert a few Wikipedia in-project interwiki prefixes,
124125 # for testing inter-language links
125126 $this->db->insert( 'interwiki', array(
Index: branches/FileBackend/phase3/tests/phpunit/includes/media/GIFTest.php
——@@ -2,12 +2,22 @@
33 class GIFHandlerTest extends MediaWikiTestCase {
44
55 public function setUp() {
6 - $this->filePath = dirname( __FILE__ ) . '/../../data/media/';
 6+ $this->filePath = dirname( __FILE__ ) . '/../../data/media';
 7+ $this->backend = new FSFileBackend( array(
 8+ 'name' => 'localtesting',
 9+ 'lockManager' => 'nullLockManager',
 10+ 'containerPaths' => array( 'data' => $this->filePath )
 11+ ) );
 12+ $this->repo = new FSRepo( array(
 13+ 'name' => 'temp',
 14+ 'url' => 'http://localhost/thumbtest',
 15+ 'backend' => $this->backend
 16+ ) );
717 $this->handler = new GIFHandler();
818 }
919
1020 public function testInvalidFile() {
11 - $res = $this->handler->getMetadata( null, $this->filePath . 'README' );
 21+ $res = $this->handler->getMetadata( null, $this->filePath . '/README' );
1222 $this->assertEquals( GIFHandler::BROKEN_FILE, $res );
1323 }
1424 /**
——@@ -16,8 +26,7 @@
1727 * @dataProvider dataIsAnimated
1828 */
1929 public function testIsAnimanted( $filename, $expected ) {
20 - $file = UnregisteredLocalFile::newFromPath( $this->filePath . $filename,
21 - 'image/gif' );
 30+ $file = $this->dataFile( $filename, 'image/gif' );
2231 $actual = $this->handler->isAnimatedImage( $file );
2332 $this->assertEquals( $expected, $actual );
2433 }
——@@ -34,8 +43,7 @@
3544 * @dataProvider dataGetImageArea
3645 */
3746 public function testGetImageArea( $filename, $expected ) {
38 - $file = UnregisteredLocalFile::newFromPath( $this->filePath . $filename,
39 - 'image/gif' );
 47+ $file = $this->dataFile( $filename, 'image/gif' );
4048 $actual = $this->handler->getImageArea( $file, $file->getWidth(), $file->getHeight() );
4149 $this->assertEquals( $expected, $actual );
4250 }
——@@ -71,15 +79,20 @@
7280 * @dataProvider dataGetMetadata
7381 */
7482 public function testGetMetadata( $filename, $expected ) {
75 - $file = UnregisteredLocalFile::newFromPath( $this->filePath . $filename,
76 - 'image/gif' );
77 - $actual = $this->handler->getMetadata( $file, $this->filePath . $filename );
 83+ $file = $this->dataFile( $filename, 'image/gif' );
 84+ $actual = $this->handler->getMetadata( $file, "$this->filePath/$filename" );
7885 $this->assertEquals( unserialize( $expected ), unserialize( $actual ) );
7986 }
 87+
8088 public function dataGetMetadata() {
8189 return array(
8290 array( 'nonanimated.gif', 'a:4:{s:10:"frameCount";i:1;s:6:"looped";b:0;s:8:"duration";d:0.1000000000000000055511151231257827021181583404541015625;s:8:"metadata";a:2:{s:14:"GIFFileComment";a:1:{i:0;s:35:"GIF test file ⁕ Created with GIMP";}s:15:"_MW_GIF_VERSION";i:1;}}' ),
8391 array( 'animated-xmp.gif', 'a:4:{s:10:"frameCount";i:4;s:6:"looped";b:1;s:8:"duration";d:2.399999999999999911182158029987476766109466552734375;s:8:"metadata";a:5:{s:6:"Artist";s:7:"Bawolff";s:16:"ImageDescription";a:2:{s:9:"x-default";s:18:"A file to test GIF";s:5:"_type";s:4:"lang";}s:15:"SublocationDest";s:13:"The interwebs";s:14:"GIFFileComment";a:1:{i:0;s:16:"GIƒ·test·file";}s:15:"_MW_GIF_VERSION";i:1;}}' ),
8492 );
8593 }
 94+
 95+ private function dataFile( $name, $type ) {
 96+ return new UnregisteredLocalFile( false, $this->repo,
 97+ "mwstore://localtesting/data/$name", $type );
 98+ }
8699 }
Index: branches/FileBackend/phase3/tests/phpunit/includes/media/ExifRotationTest.php
——@@ -7,13 +7,19 @@
88
99 function setUp() {
1010 parent::setUp();
11 - $this->filePath = dirname( __FILE__ ) . '/../../data/media/';
1211 $this->handler = new BitmapHandler();
13 - $this->repo = new FSRepo(array(
14 - 'name' => 'temp',
15 - 'directory' => wfTempDir() . '/exif-test-' . time() . '-' . mt_rand(),
16 - 'url' => 'http://localhost/thumbtest'
17 - ));
 12+ $filePath = dirname( __FILE__ ) . '/../../data/media';
 13+ $tmpDir = wfTempDir() . '/exif-test-' . time() . '-' . mt_rand();
 14+ $this->backend = new FSFileBackend( array(
 15+ 'name' => 'localtesting',
 16+ 'lockManager' => 'nullLockManager',
 17+ 'containerPaths' => array( 'images-thumb' => $tmpDir, 'data' => $filePath )
 18+ ) );
 19+ $this->repo = new FSRepo( array(
 20+ 'name' => 'temp',
 21+ 'url' => 'http://localhost/thumbtest',
 22+ 'backend' => $this->backend
 23+ ) );
1824 if ( !wfDl( 'exif' ) ) {
1925 $this->markTestSkipped( "This test needs the exif extension." );
2026 }
——@@ -39,7 +45,7 @@
4046 if ( !BitmapHandler::canRotate() ) {
4147 $this->markTestSkipped( "This test needs a rasterizer that can auto-rotate." );
4248 }
43 - $file = UnregisteredLocalFile::newFromPath( $this->filePath . $name, $type );
 49+ $file = $this->dataFile( $name, $type );
4450 $this->assertEquals( $info['width'], $file->getWidth(), "$name: width check" );
4551 $this->assertEquals( $info['height'], $file->getHeight(), "$name: height check" );
4652 }
——@@ -66,13 +72,13 @@
6773 throw new MWException('bogus test data format ' . $size);
6874 }
6975
70 - $file = $this->localFile( $name, $type );
 76+ $file = $this->dataFile( $name, $type );
7177 $thumb = $file->transform( $params, File::RENDER_NOW );
7278
7379 $this->assertEquals( $out[0], $thumb->getWidth(), "$name: thumb reported width check for $size" );
7480 $this->assertEquals( $out[1], $thumb->getHeight(), "$name: thumb reported height check for $size" );
7581
76 - $gis = getimagesize( $thumb->getPath() );
 82+ $gis = getimagesize( $thumb->getLocalCopyPath() );
7783 if ($out[0] > $info['width']) {
7884 // Physical image won't be scaled bigger than the original.
7985 $this->assertEquals( $info['width'], $gis[0], "$name: thumb actual width check for $size");
——@@ -84,8 +90,9 @@
8591 }
8692 }
8793
88 - private function localFile( $name, $type ) {
89 - return new UnregisteredLocalFile( false, $this->repo, $this->filePath . $name, $type );
 94+ private function dataFile( $name, $type ) {
 95+ return new UnregisteredLocalFile( false, $this->repo,
 96+ "mwstore://localtesting/data/$name", $type );
9097 }
9198
9299 function providerFiles() {
——@@ -129,7 +136,7 @@
130137 global $wgEnableAutoRotation;
131138 $wgEnableAutoRotation = false;
132139
133 - $file = UnregisteredLocalFile::newFromPath( $this->filePath . $name, $type );
 140+ $file = $this->dataFile( $name, $type );
134141 $this->assertEquals( $info['width'], $file->getWidth(), "$name: width check" );
135142 $this->assertEquals( $info['height'], $file->getHeight(), "$name: height check" );
136143
——@@ -158,13 +165,13 @@
159166 throw new MWException('bogus test data format ' . $size);
160167 }
161168
162 - $file = $this->localFile( $name, $type );
 169+ $file = $this->dataFile( $name, $type );
163170 $thumb = $file->transform( $params, File::RENDER_NOW );
164171
165172 $this->assertEquals( $out[0], $thumb->getWidth(), "$name: thumb reported width check for $size" );
166173 $this->assertEquals( $out[1], $thumb->getHeight(), "$name: thumb reported height check for $size" );
167174
168 - $gis = getimagesize( $thumb->getPath() );
 175+ $gis = getimagesize( $thumb->getLocalCopyPath() );
169176 if ($out[0] > $info['width']) {
170177 // Physical image won't be scaled bigger than the original.
171178 $this->assertEquals( $info['width'], $gis[0], "$name: thumb actual width check for $size");
——@@ -242,7 +249,7 @@
243250 array(
244251 270,
245252 array( self::TEST_HEIGHT, self::TEST_WIDTH )
246 - ),
 253+ ),
247254 );
248255 }
249256 }
Index: branches/FileBackend/phase3/tests/phpunit/includes/media/PNGTest.php
——@@ -2,12 +2,22 @@
33 class PNGHandlerTest extends MediaWikiTestCase {
44
55 public function setUp() {
6 - $this->filePath = dirname( __FILE__ ) . '/../../data/media/';
 6+ $this->filePath = dirname( __FILE__ ) . '/../../data/media';
 7+ $this->backend = new FSFileBackend( array(
 8+ 'name' => 'localtesting',
 9+ 'lockManager' => 'nullLockManager',
 10+ 'containerPaths' => array( 'data' => $this->filePath )
 11+ ) );
 12+ $this->repo = new FSRepo( array(
 13+ 'name' => 'temp',
 14+ 'url' => 'http://localhost/thumbtest',
 15+ 'backend' => $this->backend
 16+ ) );
717 $this->handler = new PNGHandler();
818 }
919
1020 public function testInvalidFile() {
11 - $res = $this->handler->getMetadata( null, $this->filePath . 'README' );
 21+ $res = $this->handler->getMetadata( null, $this->filePath . '/README' );
1222 $this->assertEquals( PNGHandler::BROKEN_FILE, $res );
1323 }
1424 /**
——@@ -16,8 +26,7 @@
1727 * @dataProvider dataIsAnimated
1828 */
1929 public function testIsAnimanted( $filename, $expected ) {
20 - $file = UnregisteredLocalFile::newFromPath( $this->filePath . $filename,
21 - 'image/png' );
 30+ $file = $this->dataFile( $filename, 'image/png' );
2231 $actual = $this->handler->isAnimatedImage( $file );
2332 $this->assertEquals( $expected, $actual );
2433 }
——@@ -34,8 +43,7 @@
3544 * @dataProvider dataGetImageArea
3645 */
3746 public function testGetImageArea( $filename, $expected ) {
38 - $file = UnregisteredLocalFile::newFromPath( $this->filePath . $filename,
39 - 'image/png' );
 47+ $file = $this->dataFile($filename, 'image/png' );
4048 $actual = $this->handler->getImageArea( $file, $file->getWidth(), $file->getHeight() );
4149 $this->assertEquals( $expected, $actual );
4250 }
——@@ -73,9 +81,8 @@
7482 * @dataProvider dataGetMetadata
7583 */
7684 public function testGetMetadata( $filename, $expected ) {
77 - $file = UnregisteredLocalFile::newFromPath( $this->filePath . $filename,
78 - 'image/png' );
79 - $actual = $this->handler->getMetadata( $file, $this->filePath . $filename );
 85+ $file = $this->dataFile( $filename, 'image/png' );
 86+ $actual = $this->handler->getMetadata( $file, "$this->filePath/$filename" );
8087 // $this->assertEquals( unserialize( $expected ), unserialize( $actual ) );
8188 $this->assertEquals( ( $expected ), ( $actual ) );
8289 }
——@@ -85,4 +92,9 @@
8693 array( 'xmp.png', 'a:6:{s:10:"frameCount";i:0;s:9:"loopCount";i:1;s:8:"duration";d:0;s:8:"bitDepth";i:1;s:9:"colorType";s:14:"index-coloured";s:8:"metadata";a:2:{s:12:"SerialNumber";s:9:"123456789";s:15:"_MW_PNG_VERSION";i:1;}}' ),
8794 );
8895 }
 96+
 97+ private function dataFile( $name, $type ) {
 98+ return new UnregisteredLocalFile( false, $this->repo,
 99+ "mwstore://localtesting/data/$name", $type );
 100+ }
89101 }
Index: branches/FileBackend/phase3/tests/phpunit/includes/media/FormatMetadataTest.php
——@@ -4,6 +4,17 @@
55 if ( !wfDl( 'exif' ) ) {
66 $this->markTestSkipped( "This test needs the exif extension." );
77 }
 8+ $filePath = dirname( __FILE__ ) . '/../../data/media';
 9+ $this->backend = new FSFileBackend( array(
 10+ 'name' => 'localtesting',
 11+ 'lockManager' => 'nullLockManager',
 12+ 'containerPaths' => array( 'data' => $filePath )
 13+ ) );
 14+ $this->repo = new FSRepo( array(
 15+ 'name' => 'temp',
 16+ 'url' => 'http://localhost/thumbtest',
 17+ 'backend' => $this->backend
 18+ ) );
819 global $wgShowEXIF;
920 $this->show = $wgShowEXIF;
1021 $wgShowEXIF = true;
——@@ -14,8 +25,7 @@
1526 }
1627
1728 public function testInvalidDate() {
18 - $file = UnregisteredLocalFile::newFromPath( dirname( __FILE__ ) .
19 - '/../../data/media/broken_exif_date.jpg', 'image/jpeg' );
 29+ $file = $this->dataFile( 'broken_exif_date.jpg', 'image/jpeg' );
2030
2131 // Throws an error if bug hit
2232 $meta = $file->formatMetadata();
——@@ -34,4 +44,9 @@
3545 $meta['visible'][$dateIndex]['value'],
3646 'File with invalid date metadata (bug 29471)' );
3747 }
 48+
 49+ private function dataFile( $name, $type ) {
 50+ return new UnregisteredLocalFile( false, $this->repo,
 51+ "mwstore://localtesting/data/$name", $type );
 52+ }
3853 }
Index: branches/FileBackend/phase3/tests/phpunit/includes/filerepo/FileBackendTest.php
——@@ -287,7 +287,7 @@
288288 }
289289
290290 /**
291 - * @dataProvider provider_testConcatenate
 291+ * @dataProvider provider_testCreate
292292 */
293293 public function provider_testCreate() {
294294 $cases = array();
——@@ -324,47 +324,17 @@
325325 return $cases;
326326 }
327327
328 - /**
329 - * @dataProvider provider_testConcatenate
330 - */
331 - public function testConcatenate() {
332 -
333 - }
 328+ // @TODO: testConcatenate
334329
335 - /**
336 - * @dataProvider provider_testPrepare
337 - */
338 - public function testPrepare() {
339 -
340 - }
 330+ // @TODO: testPrepare
341331
342 - /**
343 - * @dataProvider provider_testSecure
344 - */
345 - public function testSecure() {
346 -
347 - }
 332+ // @TODO: testSecure
348333
349 - /**
350 - * @dataProvider provider_testClean
351 - */
352 - public function testClean() {
353 -
354 - }
 334+ // @TODO: testClean
355335
356 - /**
357 - * @dataProvider provider_testGetLocalCopy
358 - */
359 - public function testGetLocalCopy() {
360 -
361 - }
 336+ // @TODO: testGetLocalCopy
362337
363 - /**
364 - * @dataProvider provider_testDoOperations
365 - */
366 - public function testDoOperations() {
367 -
368 - }
 338+ // @TODO: testDoOperations
369339
370340 public function testGetFileList() {
371341 $base = $this->singleBasePath();
Index: branches/FileBackend/phase3/includes/AutoLoader.php
——@@ -499,6 +499,7 @@
500500 'LockManager' => 'includes/filerepo/backend/LockManager.php',
501501 'FSLockManager' => 'includes/filerepo/backend/LockManager.php',
502502 'DBLockManager' => 'includes/filerepo/backend/LockManager.php',
 503+ 'NullLockManager' => 'includes/filerepo/backend/LockManager.php',
503504 'FileOp' => 'includes/filerepo/backend/FileOp.php',
504505 'StoreFileOp' => 'includes/filerepo/backend/FileOp.php',
505506 'CopyFileOp' => 'includes/filerepo/backend/FileOp.php',
Index: branches/FileBackend/phase3/includes/filerepo/backend/LockManager.php
——@@ -260,6 +260,7 @@
261261 * All lock requests for a resource, identified by a hash string, will
262262 * map to one bucket. Each bucket maps to one or several peer DB servers,
263263 * each having a `file_locks` table with row-level locking.
 264+ * This does not use GET_LOCK() per http://bugs.mysql.com/bug.php?id=1118.
264265 *
265266 * A majority of peer servers must agree for a lock to be acquired.
266267 * As long as one peer server is up, lock requests will not be blocked
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php
——@@ -289,9 +289,7 @@
290290 * @param $params Array
291291 * @return FSFile|null Returns null on failure
292292 */
293 - public function getLocalReference( array $params ) {
294 - return $this->getLocalCopy( $params );
295 - }
 293+ abstract public function getLocalReference( array $params );
296294
297295 /**
298296 * Get a local copy on disk of the file at a storage path in the backend.
——@@ -477,6 +475,10 @@
478476 return 'internal';
479477 }
480478
 479+ public function getLocalReference( array $params ) {
 480+ return $this->getLocalCopy( $params );
 481+ }
 482+
481483 function streamFile( array $params ) {
482484 $status = Status::newGood();
483485
——@@ -736,7 +738,7 @@
737739 // This accounts for Swift and S3 restrictions. Also note
738740 // that these urlencode to the same string, which is useful
739741 // since the Swift size limit is *after* URL encoding.
740 - return preg_match( '/^[a-zA-Z._-]{1,256}$/u', $container );
 742+ return preg_match( '/^[a-zA-Z0-9._-]{1,256}$/u', $container );
741743 }
742744
743745 /**
Index: branches/FileBackend/phase3/includes/filerepo/FileRepo.php
——@@ -44,7 +44,11 @@
4545 $this->url = isset( $info['url'] )
4646 ? $info['url']
4747 : false; // a subclass will need to set the URL (e.g. ForeignAPIRepo)
48 - $this->backend = FileBackendGroup::singleton()->get( $info['backend'] );
 48+ if ( $info['backend'] instanceof FileBackendBase ) {
 49+ $this->backend = $info['backend']; // useful for testing
 50+ } else {
 51+ $this->backend = FileBackendGroup::singleton()->get( $info['backend'] );
 52+ }
4953
5054 // Optional settings that can have no value
5155 $optionalSettings = array(
——@@ -94,7 +98,7 @@
9599 }
96100
97101 /**
98 - * Get the file backend
 102+ * Get the file backend instance
99103 *
100104 * @return FileBackendBase
101105 */
Index: branches/FileBackend/phase3/includes/media/MediaTransformOutput.php
——@@ -78,8 +78,8 @@
7979 * @return Bool
8080 */
8181 public function hasFile() {
82 - // If TRANSFORM_LATER, a 0-byte temp file be at the path (not purged yet)
83 - return ( !$this->isError() && $this->path && filesize( $this->path ) );
 82+ // If TRANSFORM_LATER, $this->path will be false
 83+ return ( !$this->isError() && $this->path );
8484 }
8585
8686 /**
——@@ -93,6 +93,15 @@
9494 }
9595
9696 /**
 97+ * Get the path of a file system copy of the thumbnail
 98+ *
 99+ * @return string|false Returns false if there isn't one
 100+ */
 101+ public function getLocalCopyPath() {
 102+ return $this->path;
 103+ }
 104+
 105+ /**
97106 * Stream the file if there were no errors
98107 *
99108 * @param $headers Array Additional HTTP headers to send on success
Index: branches/FileBackend/phase3/includes/media/Generic.php
——@@ -216,7 +216,7 @@
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() && $out->hasFile() ) {
 220+ if ( !$out->isError() && $out->hasFile() && !$out->fileIsSource() ) {
221221 $op = array( 'op' => 'store',
222222 'src' => $tmpDest, 'dst' => $dstPath, 'overwriteDest' => true );
223223 if ( !$image->getRepo()->getBackend()->doOperation( $op )->isOK() ) {

Status & tagging log