r104366 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r104365‎ \| r104366 \| r104367 >
Date:	21:48, 27 November 2011
Author:	aaron
Status:	deferred
Tags:
Comment:	* Renamed resolveVirtualPath() -> resolveStoragePath() * Added splitStoragePath() function * Renamed storagePathsToLock() -> storagePathsUsed() * Added FileBackendGroup class to handle registration * Made File::getTimestamp() use FileBackend::getFileTimestamp() * Replaced unlink() in LocalFile::cleanupDeletedBatch()
Modified paths:	/branches/FileBackend/phase3/includes/AutoLoader.php (modified) (history) /branches/FileBackend/phase3/includes/filerepo/FileRepo.php (modified) (history) /branches/FileBackend/phase3/includes/filerepo/LocalRepo.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/FileBackendGroup.php (added) (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/file/File.php (modified) (history)

Diff [purge]

Index: branches/FileBackend/phase3/includes/filerepo/file/File.php
—	—	@@ -835,10 +835,10 @@
836	836	global $wgStylePath, $wgStyleDirectory;
837	837
838	838	$try = array( 'fileicon-' . $this->getExtension() . '.png', 'fileicon.png' );
839		~~- foreach( $try as $icon ) {~~
	839	+ foreach ( $try as $icon ) {
840	840	$path = '/common/images/icons/' . $icon;
841	841	$filepath = $wgStyleDirectory . $path;
842		~~- if( file_exists( $filepath ) ) {~~
	842	+ if ( file_exists( $filepath ) ) { // always FS
843	843	return new ThumbnailImage( $this, $wgStylePath . $path, 120, 120 );
844	844	}
845	845	}
—	—	@@ -1450,17 +1450,13 @@
1451	1451	}
1452	1452
1453	1453	/**
1454		~~- * Get the 14-character timestamp of the file upload, or false if~~
1455		~~- * it doesn't exist~~
	1454	+ * Get the 14-character timestamp of the file upload
1456	1455	*
1457		~~- * @return string~~
	1456	+ * @return string\|false TS_MW timestamp or false on failure
1458	1457	*/
1459	1458	function getTimestamp() {
1460		~~- $path = $this->getPath();~~
1461		~~- if ( !$this->repo->fileExists( $path ) ) {~~
1462		~~- return false;~~
1463		~~- }~~
1464		~~- return wfTimestamp( TS_MW, filemtime( $path ) );~~
	1459	+ $this->assertRepoDefined();
	1460	+ return $this->repo->getBackend()->getFileTimestamp( $this->getPath() );
1465	1461	}
1466	1462
1467	1463	/**
Index: branches/FileBackend/phase3/includes/filerepo/LocalRepo.php
—	—	@@ -56,6 +56,7 @@
57	57	* @return FileRepoStatus
58	58	*/
59	59	function cleanupDeletedBatch( $storageKeys ) {
	60	+ $backend = $this->backend; // convenience
60	61	$root = $this->getZonePath( 'deleted' );
61	62	$dbw = $this->getMasterDB();
62	63	$status = $this->newGood();
—	—	@@ -70,10 +71,8 @@
71	72	$hidden = $this->hiddenFileHasKey( $key, 'lock' );
72	73	if ( !$deleted && !$hidden ) { // not in use now
73	74	wfDebug( __METHOD__ . ": deleting $key\n" );
74		~~- wfSuppressWarnings();~~
75		~~- $unlink = unlink( $path );~~
76		~~- wfRestoreWarnings();~~
77		~~- if ( !$unlink ) {~~
	75	+ $op = array( 'operation' => 'delete', 'source' => $path );
	76	+ if ( !$backend->doOperations( array( $op ) )->isOK() ) {
78	77	$status->error( 'undelete-cleanup-error', $path );
79	78	$status->failCount++;
80	79	}
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackendMultiWrite.php
—	—	@@ -81,7 +81,7 @@
82	82	// locks two or three times (depending on the number of backends).
83	83	if ( $index == 0 ) {
84	84	foreach ( $performOps as $index => $fileOp ) {
85		~~- $filesToLock = array_merge( $filesToLock, $fileOp->storagePathsToLock() );~~
	85	+ $filesToLock = array_merge( $filesToLock, $fileOp->storagePathsUsed() );
86	86	}
87	87	}
88	88	}
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileOp.php
—	—	@@ -117,7 +117,7 @@
118	118	*
119	119	* @return Array
120	120	*/
121		~~- public function storagePathsToLock() {~~
	121	+ public function storagePathsUsed() {
122	122	return array();
123	123	}
124	124
—	—	@@ -301,7 +301,7 @@
302	302	return md5_file( $this->params['source'] );
303	303	}
304	304
305		~~- function storagePathsToLock() {~~
	305	+ function storagePathsUsed() {
306	306	return array( $this->params['dest'] );
307	307	}
308	308	}
—	—	@@ -347,7 +347,7 @@
348	348	return md5( $this->params['content'] );
349	349	}
350	350
351		~~- function storagePathsToLock() {~~
	351	+ function storagePathsUsed() {
352	352	return array( $this->params['dest'] );
353	353	}
354	354	}
—	—	@@ -400,7 +400,7 @@
401	401	return $this->getFileMD5( $this->params['source'] );
402	402	}
403	403
404		~~- function storagePathsToLock() {~~
	404	+ function storagePathsUsed() {
405	405	return array( $this->params['source'], $this->params['dest'] );
406	406	}
407	407	}
—	—	@@ -485,7 +485,7 @@
486	486	return $this->getFileMD5( $this->params['source'] );
487	487	}
488	488
489		~~- function storagePathsToLock() {~~
	489	+ function storagePathsUsed() {
490	490	return array( $this->params['source'], $this->params['dest'] );
491	491	}
492	492	}
—	—	@@ -531,7 +531,7 @@
532	532	return null; // defer this until we finish building the new file
533	533	}
534	534
535		~~- function storagePathsToLock() {~~
	535	+ function storagePathsUsed() {
536	536	return array_merge( $this->params['sources'], $this->params['dest'] );
537	537	}
538	538	}
—	—	@@ -569,7 +569,7 @@
570	570	return $status;
571	571	}
572	572
573		~~- function storagePathsToLock() {~~
	573	+ function storagePathsUsed() {
574	574	return array( $this->params['source'] );
575	575	}
576	576	}
Index: branches/FileBackend/phase3/includes/filerepo/backend/FSFileBackend.php
—	—	@@ -37,7 +37,7 @@
38	38	function store( array $params ) {
39	39	$status = Status::newGood();
40	40
41		~~- list( $c, $dest ) = $this->resolveVirtualPath( $params['dest'] );~~
	41	+ list( $c, $dest ) = $this->resolveStoragePath( $params['dest'] );
42	42	if ( $dest === null ) {
43	43	$status->fatal( 'backend-fail-invalidpath', $params['dest'] );
44	44	return $status;
—	—	@@ -87,12 +87,12 @@
88	88	function move( array $params ) {
89	89	$status = Status::newGood();
90	90
91		~~- list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );~~
	91	+ list( $c, $source ) = $this->resolveStoragePath( $params['source'] );
92	92	if ( $source === null ) {
93	93	$status->fatal( 'backend-fail-invalidpath', $params['source'] );
94	94	return $status;
95	95	}
96		~~- list( $c, $dest ) = $this->resolveVirtualPath( $params['dest'] );~~
	96	+ list( $c, $dest ) = $this->resolveStoragePath( $params['dest'] );
97	97	if ( $dest === null ) {
98	98	$status->fatal( 'backend-fail-invalidpath', $params['dest'] );
99	99	return $status;
—	—	@@ -135,7 +135,7 @@
136	136	function delete( array $params ) {
137	137	$status = Status::newGood();
138	138
139		~~- list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );~~
	139	+ list( $c, $source ) = $this->resolveStoragePath( $params['source'] );
140	140	if ( $source === null ) {
141	141	$status->fatal( 'backend-fail-invalidpath', $params['source'] );
142	142	return $status;
—	—	@@ -162,7 +162,7 @@
163	163	function concatenate( array $params ) {
164	164	$status = Status::newGood();
165	165
166		~~- list( $c, $dest ) = $this->resolveVirtualPath( $params['dest'] );~~
	166	+ list( $c, $dest ) = $this->resolveStoragePath( $params['dest'] );
167	167	if ( $dest === null ) {
168	168	$status->fatal( 'backend-fail-invalidpath', $params['dest'] );
169	169	return $status;
—	—	@@ -193,7 +193,7 @@
194	194	return $status;
195	195	}
196	196	foreach ( $params['sources'] as $virtualSource ) {
197		~~- list( $c, $source ) = $this->resolveVirtualPath( $virtualSource );~~
	197	+ list( $c, $source ) = $this->resolveStoragePath( $virtualSource );
198	198	if ( $source === null ) {
199	199	$status->fatal( 'backend-fail-invalidpath', $virtualSource );
200	200	return $status;
—	—	@@ -262,7 +262,7 @@
263	263	function create( array $params ) {
264	264	$status = Status::newGood();
265	265
266		~~- list( $c, $dest ) = $this->resolveVirtualPath( $params['dest'] );~~
	266	+ list( $c, $dest ) = $this->resolveStoragePath( $params['dest'] );
267	267	if ( $dest === null ) {
268	268	$status->fatal( 'backend-fail-invalidpath', $params['dest'] );
269	269	return $status;
—	—	@@ -303,7 +303,7 @@
304	304
305	305	function prepare( array $params ) {
306	306	$status = Status::newGood();
307		~~- list( $c, $dir ) = $this->resolveVirtualPath( $params['directory'] );~~
	307	+ list( $c, $dir ) = $this->resolveStoragePath( $params['directory'] );
308	308	if ( $dir === null ) {
309	309	$status->fatal( 'backend-fail-invalidpath', $params['directory'] );
310	310	return $status; // invalid storage path
—	—	@@ -323,7 +323,7 @@
324	324
325	325	function secure( array $params ) {
326	326	$status = Status::newGood();
327		~~- list( $c, $dir ) = $this->resolveVirtualPath( $params['directory'] );~~
	327	+ list( $c, $dir ) = $this->resolveStoragePath( $params['directory'] );
328	328	if ( $dir === null ) {
329	329	$status->fatal( 'backend-fail-invalidpath', $params['directory'] );
330	330	return $status; // invalid storage path
—	—	@@ -356,7 +356,7 @@
357	357	}
358	358
359	359	function fileExists( array $params ) {
360		~~- list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );~~
	360	+ list( $c, $source ) = $this->resolveStoragePath( $params['source'] );
361	361	if ( $source === null ) {
362	362	return false; // invalid storage path
363	363	}
—	—	@@ -368,7 +368,7 @@
369	369	}
370	370
371	371	function getFileHash( array $params ) {
372		~~- list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );~~
	372	+ list( $c, $source ) = $this->resolveStoragePath( $params['source'] );
373	373	if ( $source === null ) {
374	374	return false; // invalid storage path
375	375	}
—	—	@@ -376,7 +376,7 @@
377	377	}
378	378
379	379	function getFileTimestamp( array $params ) {
380		~~- list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );~~
	380	+ list( $c, $source ) = $this->resolveStoragePath( $params['source'] );
381	381	if ( $source === null ) {
382	382	return false; // invalid storage path
383	383	}
—	—	@@ -385,7 +385,7 @@
386	386	}
387	387
388	388	function getFileProps( array $params ) {
389		~~- list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );~~
	389	+ list( $c, $source ) = $this->resolveStoragePath( $params['source'] );
390	390	if ( $source === null ) {
391	391	return FSFile::placeholderProps(); // invalid storage path
392	392	}
—	—	@@ -394,7 +394,7 @@
395	395	}
396	396
397	397	function getFileList( array $params ) {
398		~~- list( $c, $dir ) = $this->resolveVirtualPath( $params['directory'] );~~
	398	+ list( $c, $dir ) = $this->resolveStoragePath( $params['directory'] );
399	399	if ( $dir === null ) { // invalid storage path
400	400	return array(); // empty result
401	401	}
—	—	@@ -404,7 +404,7 @@
405	405	function streamFile( array $params ) {
406	406	$status = Status::newGood();
407	407
408		~~- list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );~~
	408	+ list( $c, $source ) = $this->resolveStoragePath( $params['source'] );
409	409	if ( $source === null ) {
410	410	$status->fatal( 'backend-fail-invalidpath', $params['source'] );
411	411	return $status;
—	—	@@ -420,7 +420,7 @@
421	421	}
422	422
423	423	function getLocalCopy( array $params ) {
424		~~- list( $c, $source ) = $this->resolveVirtualPath( $params['source'] );~~
	424	+ list( $c, $source ) = $this->resolveStoragePath( $params['source'] );
425	425	if ( $source === null ) {
426	426	return null;
427	427	}
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackendGroup.php
—	—	@@ -0,0 +1,88 @@
	2	+<?php
	3	+/**
	4	+ * @file
	5	+ * @ingroup FileRepo
	6	+ * @ingroup FileBackend
	7	+ */
	8	+
	9	+/**
	10	+ * Class to handle file backend registration
	11	+ *
	12	+ * @ingroup FileRepo
	13	+ * @ingroup FileBackend
	14	+ */
	15	+class FileBackendGroup {
	16	+ protected static $instance = null;
	17	+
	18	+ /** @var Array of (name => ('class' =>, 'config' =>, 'instance' =>)) */
	19	+ protected $backends = array();
	20	+
	21	+ protected function __construct() {}
	22	+ protected function __clone() {}
	23	+
	24	+ public static function singleton() {
	25	+ if ( self::$instance == null ) {
	26	+ self::$instance = new self();
	27	+ }
	28	+ return self::$instance;
	29	+ }
	30	+
	31	+ /**
	32	+ * Register a file backend from configuration
	33	+ *
	34	+ * @param $config Array
	35	+ * @return void
	36	+ * @throws MWException
	37	+ */
	38	+ public function register( array $config ) {
	39	+ if ( !isset( $config['name'] ) ) {
	40	+ throw new MWException( "Cannot register a backend with no name." );
	41	+ }
	42	+ $name = $config['name'];
	43	+ if ( !isset( $config['class'] ) ) {
	44	+ throw new MWException( "Cannot register backend `{$name}` with no class." );
	45	+ }
	46	+ $class = $config['class'];
	47	+
	48	+ unset( $config['class'] ); // backend won't need this
	49	+ $this->backends[$name] = array(
	50	+ 'class' => $class,
	51	+ 'config' => $config,
	52	+ 'instance' => null
	53	+ );
	54	+ }
	55	+
	56	+ /**
	57	+ * Get the backend object with a given name
	58	+ *
	59	+ * @param $name string
	60	+ * @return FileBackendBase
	61	+ * @throws MWException
	62	+ */
	63	+ public function getBackend( $name ) {
	64	+ if ( !isset( $this->backends[$name] ) ) {
	65	+ throw new MWException( "No backend defined with the name `$name`." );
	66	+ }
	67	+ // Lazy-load the actual backend instance
	68	+ if ( !isset( $this->backends[$name]['instance'] ) ) {
	69	+ $class = $this->backends[$name]['class'];
	70	+ $config = $this->backends[$name]['config'];
	71	+ $this->backends[$name]['instance'] = new $class( $config );
	72	+ }
	73	+ return $this->backends[$name]['instance'];
	74	+ }
	75	+
	76	+ /**
	77	+ * Get an appropriate backend object from a storage path
	78	+ *
	79	+ * @param $storagePath string
	80	+ * @return FileBackendBase\|null Backend or null on failure
	81	+ */
	82	+ public function backendFromPath( $storagePath ) {
	83	+ list( $backend, $c, $p ) = FileBackend::splitStoragePath( $storagePath );
	84	+ if ( $backend !== null && isset( $this->backends[$backend] ) ) {
	85	+ return $this->backends[$backend];
	86	+ }
	87	+ return null;
	88	+ }
	89	+}
Property changes on: branches/FileBackend/phase3/includes/filerepo/backend/FileBackendGroup.php
___________________________________________________________________
Added: svn:eol-style
1	90	+ native
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php
—	—	@@ -392,7 +392,7 @@
393	393	// Build up a list of files to lock...
394	394	$filesToLock = array();
395	395	foreach ( $performOps as $index => $fileOp ) {
396		~~- $filesToLock = array_merge( $filesToLock, $fileOp->storagePathsToLock() );~~
	396	+ $filesToLock = array_merge( $filesToLock, $fileOp->storagePathsUsed() );
397	397	}
398	398
399	399	// Try to lock those files...
—	—	@@ -481,21 +481,37 @@
482	482
483	483	/**
484	484	* Split a storage path (e.g. "mwstore://backend/container/path/to/object")
485		~~- * into a container name and a full object name within that container.~~
	485	+ * into a backend name, a container name, and a relative object path.
486	486	*
487	487	* @param $storagePath string
	488	+ * @return Array (backend, container, rel object) or (null, null, null)
	489	+ */
	490	+ final public static function splitStoragePath( $storagePath ) {
	491	+ if ( self::isStoragePath( $storagePath ) ) {
	492	+ // Note: strlen( 'mwstore://' ) = 10
	493	+ $parts = explode( '/', substr( $storagePath, 10 ), 3 );
	494	+ if ( count( $parts ) == 3 ) {
	495	+ return $parts;
	496	+ }
	497	+ }
	498	+ return array( null, null, null );
	499	+ }
	500	+
	501	+ /**
	502	+ * Split a storage path (e.g. "mwstore://backend/container/path/to/object")
	503	+ * into a backend name, a container name and an internal relative object name.
	504	+ *
	505	+ * @param $storagePath string
488	506	* @return Array (container, object name) or (null, null) if path is invalid
489	507	*/
490		~~- final protected function resolveVirtualPath( $storagePath ) {~~
491		~~- if ( strpos( $storagePath, 'mwstore://' ) === 0 ) {~~
492		~~- $m = explode( '/', substr( $storagePath, 10 ), 3 );~~
493		~~- if ( count( $m ) == 3 ) {~~
494		~~- list( $backend, $container, $relPath ) = $m;~~
495		~~- if ( $backend === $this->name ) { // sanity~~
496		~~- $relPath = $this->resolveContainerPath( $container, $relPath );~~
497		~~- if ( $relPath !== null ) {~~
498		~~- return array( $container, $relPath ); // (container, path)~~
499		~~- }~~
	508	+ final protected function resolveStoragePath( $storagePath ) {
	509	+ $parts = self::splitStoragePath( $storagePath );
	510	+ if ( $parts[0] !== null ) { // either all null or all not null
	511	+ list( $backend, $container, $relPath ) = $parts;
	512	+ if ( $backend === $this->name ) { // sanity
	513	+ $relPath = $this->resolveContainerPath( $container, $relPath );
	514	+ if ( $relPath !== null ) {
	515	+ return array( $container, $relPath ); // (container, path)
500	516	}
501	517	}
502	518	}
Index: branches/FileBackend/phase3/includes/filerepo/FileRepo.php
—	—	@@ -753,7 +753,7 @@
754	754	// Cleanup for disk source files...
755	755	foreach ( $sourceFSFilesToDelete as $file ) {
756	756	wfSuppressWarnings();
757		~~- unlink( $file );~~
	757	+ unlink( $file ); // FS cleanup
758	758	wfRestoreWarnings();
759	759	}
760	760
—	—	@@ -802,7 +802,7 @@
803	803	// Cleanup for disk source files...
804	804	foreach ( $sourceFSFilesToDelete as $file ) {
805	805	wfSuppressWarnings();
806		~~- unlink( $path );~~
	806	+ unlink( $path ); // FS cleanup
807	807	wfRestoreWarnings();
808	808	}
809	809	}
—	—	@@ -964,7 +964,7 @@
965	965	// Cleanup for disk source files...
966	966	foreach ( $sourceFSFilesToDelete as $file ) {
967	967	wfSuppressWarnings();
968		~~- unlink( $file );~~
	968	+ unlink( $file ); // FS cleanup
969	969	wfRestoreWarnings();
970	970	}
971	971
—	—	@@ -1005,9 +1005,9 @@
1006	1006	$result[$key] = $this->backend->fileExists( array( 'source' => $file ) );
1007	1007	} else {
1008	1008	if ( $flags & self::FILES_ONLY ) {
1009		~~- $result[$key] = is_file( $file );~~
	1009	+ $result[$key] = is_file( $file ); // FS only
1010	1010	} else {
1011		~~- $result[$key] = file_exists( $file ); // @TODO: kill this~~
	1011	+ $result[$key] = file_exists( $file ); // FS only
1012	1012	}
1013	1013	}
1014	1014	}
Index: branches/FileBackend/phase3/includes/AutoLoader.php
—	—	@@ -484,6 +484,7 @@
485	485	'TempFSFile' => 'includes/filerepo/file/TempFSFile.php',
486	486
487	487	# includes/filerepo/backend
	488	+ 'FileBackendGroup' => 'includes/filerepo/backend/FileBackendGroup',
488	489	'FileBackendBase' => 'includes/filerepo/backend/FileBackend.php',
489	490	'FileBackend' => 'includes/filerepo/backend/FileBackend.php',
490	491	'FileBackendMultiWrite' => 'includes/filerepo/backend/FileBackendMultiWrite.php',

Status & tagging log

23:51, 7 December 2011 Siebrand (talk | contribs) changed the status of r104366 [removed: new added: deferred]