r103744 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r103743‎ \| r103744 \| r103745 >
Date:	10:42, 20 November 2011
Author:	aaron
Status:	deferred
Tags:
Comment:	* Added stream() function to backends * Moved TempLocalFile.php under file/ and simplified the class a bit * Changed getLocalCopy() to return a TempLocalFile * Removed fullStoragePath(), left in by accident * Removed some redundant function visibility in FSFileBackend/FileBackendMultiWrite (defined by interface/base already) * Other cleanups
Modified paths:	/branches/FileBackend/phase3/includes/filerepo/TempLocalFile.php (deleted) (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/FileLockManager.php (modified) (history) /branches/FileBackend/phase3/includes/filerepo/file/File.php (modified) (history) /branches/FileBackend/phase3/includes/filerepo/file/TempLocalFile.php (added) (history)

Diff [purge]

Index: branches/FileBackend/phase3/includes/filerepo/TempLocalFile.php
—	—	@@ -1,60 +0,0 @@
2		~~-<?php~~
3		-
4		-/**
5		~~- * This class is used to hold the location to and do limited manipulation of~~
6		~~- * files stored temporarily (usually this will be $wgTmpDirectory)~~
7		- *
8		~~- */~~
9		~~-class TempLocalFile {~~
10		-
11		- /**
12		~~- * Path to local temp file directory~~
13		~~- * @var String~~
14		~~- */~~
15		~~- private $path;~~
16		-
17		- /**
18		~~- * Does this object have authority to remove the temp file~~
19		~~- * @var Boolean~~
20		~~- */~~
21		~~- private $canDelete;~~
22		-
23		- /**
24		~~- * Sets up the temporary file object~~
25		~~- * @param String $path - Path to temporary file on local disk~~
26		~~- * @param Boolean $canDelete - Does this object have authority to remove the file~~
27		~~- */~~
28		~~- public function __construct($path, $canDelete = true) {~~
29		~~- $this->path = $path;~~
30		~~- $this->canDelete = $canDelete;~~
31		~~- }~~
32		-
33		- /**
34		~~- * Returns the local path to the temp file~~
35		~~- * @return String~~
36		~~- */~~
37		~~- public function getPath() {~~
38		~~- return $this->path;~~
39		~~- }~~
40		-
41		- /**
42		~~- * If this object has the authority this method will remove the temp file~~
43		~~- * from the local disk. This method is called on object destruct, but~~
44		~~- * may also be called any time a cleanup is needed~~
45		~~- */~~
46		~~- public function cleanup() {~~
47		~~- if($this->canDelete) {~~
48		~~- unlink($this->path);~~
49		~~- }~~
50		~~- }~~
51		-
52		- /**
53		~~- * Object destructor.~~
54		- *
55		~~- * Cleans up after the temporary file. Currently this means removing it~~
56		~~- * from the local disk~~
57		~~- */~~
58		~~- public function __destruct() {~~
59		~~- $this->cleanup();~~
60		~~- }~~
61		~~-} // end class~~
\ No newline at end of file
Property changes on: branches/FileBackend/phase3/includes/filerepo/file/File.php
___________________________________________________________________
Modified: svn:mergeinfo
62	1	Merged /trunk/phase3/includes/filerepo/file/File.php:r103742
Index: branches/FileBackend/phase3/includes/filerepo/file/TempLocalFile.php
—	—	@@ -0,0 +1,46 @@
	2	+<?php
	3	+/**
	4	+ * @file
	5	+ * @ingroup FileRepo
	6	+ */
	7	+
	8	+/**
	9	+ * This class is used to hold the location and do limited manipulation
	10	+ * of files stored temporarily (usually this will be $wgTmpDirectory)
	11	+ */
	12	+class TempLocalFile {
	13	+ protected $path; // path to local temp file directory
	14	+ protected $canDelete; // whether this object should garbage collect the temp file
	15	+
	16	+ /**
	17	+ * Sets up the temporary file object
	18	+ *
	19	+ * @param String $path Path to temporary file on local disk
	20	+ * @param Boolean $canDelete Whether this object should garbage collect the temp file
	21	+ */
	22	+ public function __construct( $path, $canDelete = true ) {
	23	+ $this->path = $path;
	24	+ $this->canDelete = $canDelete;
	25	+ }
	26	+
	27	+ /**
	28	+ * Returns the local path to the temp file
	29	+ *
	30	+ * @return String
	31	+ */
	32	+ public function getPath() {
	33	+ return $this->path;
	34	+ }
	35	+
	36	+ /**
	37	+ * Cleans up after the temporary file.
	38	+ * Currently this means removing it from the local disk.
	39	+ */
	40	+ protected function __destruct() {
	41	+ if ( $this->canDelete ) {
	42	+ wfSuppressWarnings();
	43	+ unlink( $this->path );
	44	+ wfRestoreWarnings();
	45	+ }
	46	+ }
	47	+}
Property changes on: branches/FileBackend/phase3/includes/filerepo/file/TempLocalFile.php
___________________________________________________________________
Added: svn:eol-style
1	48	+ native
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackendMultiWrite.php
—	—	@@ -28,9 +28,10 @@
29	29
30	30	public function __construct( array $config ) {
31	31	$this->name = $config['name'];
	32	+ $this->fileBackends = $config['backends'];
32	33	}
33	34
34		~~- public function getName() {~~
	35	+ function getName() {
35	36	return $this->name;
36	37	}
37	38
—	—	@@ -72,32 +73,32 @@
73	74	return $status;
74	75	}
75	76
76		~~- public function store( array $params ) {~~
	77	+ function store( array $params ) {
77	78	$op = array( 'operation' => 'store' ) + $params;
78	79	return $this->doOperation( array( $op ) );
79	80	}
80	81
81		~~- public function copy( array $params ) {~~
	82	+ function copy( array $params ) {
82	83	$op = array( 'operation' => 'copy' ) + $params;
83	84	return $this->doOperation( array( $op ) );
84	85	}
85	86
86		~~- public function move( array $params ) {~~
	87	+ function move( array $params ) {
87	88	$op = array( 'operation' => 'move' ) + $params;
88	89	return $this->doOperation( array( $op ) );
89	90	}
90	91
91		~~- public function delete( array $params ){~~
	92	+ function delete( array $params ){
92	93	$op = array( 'operation' => 'delete' ) + $params;
93	94	return $this->doOperation( array( $op ) );
94	95	}
95	96
96		~~- public function concatenate( array $params ){~~
	97	+ function concatenate( array $params ){
97	98	$op = array( 'operation' => 'concatenate' ) + $params;
98	99	return $this->doOperation( array( $op ) );
99	100	}
100	101
101		~~- public function fileExists( array $params ) {~~
	102	+ function fileExists( array $params ) {
102	103	foreach ( $this->backends as $backend ) {
103	104	if ( $backend->fileExists( $params ) ) {
104	105	return true;
—	—	@@ -105,28 +106,43 @@
106	107	}
107	108	return false;
108	109	}
109		-
110		~~- public function getLocalCopy( array $params ) {~~
	110	+
	111	+ function getFileProps( array $params ) {
111	112	foreach ( $this->backends as $backend ) {
112		~~- $tmpPath = $backend->getLocalCopy( $params );~~
113		~~- if ( $tmpPath !== null ) {~~
114		~~- return $tmpPath;~~
	113	+ $props = $backend->getFileProps( $params );
	114	+ if ( $props !== null ) {
	115	+ return $props;
115	116	}
116	117	}
117	118	return null;
118	119	}
119		-
120		~~- public function getFileProps( array $params ) {~~
	120	+
	121	+ function streamFile( array $params ) {
	122	+ if ( !count( $this->backends ) ) {
	123	+ return Status::newFatal( "No file backends are defined." );
	124	+ }
121	125	foreach ( $this->backends as $backend ) {
122		~~- $props = $backend->getFileProps( $params );~~
123		~~- if ( $props !== null ) {~~
124		~~- return $props;~~
	126	+ $status = $backend->streamFile( $params );
	127	+ if ( $status->isOK() ) {
	128	+ return $status;
	129	+ } else {
	130	+ // @TODO: check if we failed mid-stream and return out if so
125	131	}
126	132	}
	133	+ return Status::newFatal( "Could not stream file {$params['source']}." );
	134	+ }
	135	+
	136	+ function getLocalCopy( array $params ) {
	137	+ foreach ( $this->backends as $backend ) {
	138	+ $tmpFile = $backend->getLocalCopy( $params );
	139	+ if ( $tmpFile ) {
	140	+ return $tmpFile;
	141	+ }
	142	+ }
127	143	return null;
128	144	}
129	145
130		~~- public function lockFiles( array $paths ) {~~
	146	+ function lockFiles( array $paths ) {
131	147	$status = Status::newGood();
132	148	foreach ( $this->backends as $index => $backend ) {
133	149	$subStatus = $backend->lockFiles( $paths );
—	—	@@ -143,7 +159,7 @@
144	160	return $status;
145	161	}
146	162
147		~~- public function unlockFiles( array $paths ) {~~
	163	+ function unlockFiles( array $paths ) {
148	164	$status = Status::newGood();
149	165	foreach ( $this->backends as $backend ) {
150	166	$subStatus = $backend->unlockFile( $paths );
Index: branches/FileBackend/phase3/includes/filerepo/backend/FSFileBackend.php
—	—	@@ -18,7 +18,7 @@
19	19	: 0644;
20	20	}
21	21
22		~~- public function store( array $params ) {~~
	22	+ function store( array $params ) {
23	23	$status = Status::newGood();
24	24
25	25	if ( file_exists( $params['dest'] ) ) {
—	—	@@ -54,11 +54,11 @@
55	55	return $status;
56	56	}
57	57
58		~~- public function copy( array $params ) {~~
	58	+ function copy( array $params ) {
59	59	return $this->store( $params ); // both source and dest are on FS
60	60	}
61	61
62		~~- public function move( array $params ) {~~
	62	+ function move( array $params ) {
63	63	$status = Status::newGood();
64	64
65	65	if ( file_exists( $params['dest'] ) ) {
—	—	@@ -92,7 +92,7 @@
93	93	return $status;
94	94	}
95	95
96		~~- public function delete( array $params ) {~~
	96	+ function delete( array $params ) {
97	97	$status = Status::newGood();
98	98
99	99	if ( !file_exists( $params['source'] ) ) {
—	—	@@ -113,7 +113,7 @@
114	114	return $status;
115	115	}
116	116
117		~~- public function concatenate( array $params ) {~~
	117	+ function concatenate( array $params ) {
118	118	$status = Status::newGood();
119	119
120	120	// Check if the destination file exists and we can't handle that
—	—	@@ -196,15 +196,27 @@
197	197	return $status;
198	198	}
199	199
200		~~- public function fileExists( array $params ) {~~
	200	+ function fileExists( array $params ) {
201	201	return file_exists( $params['source'] );
202	202	}
203	203
204		~~- public function getFileProps( array $params ) {~~
	204	+ function getFileProps( array $params ) {
205	205	return File::getPropsFromPath( $params['source'] );
206	206	}
207	207
208		~~- public function getLocalCopy( array $params ) {~~
	208	+ function streamFile( array $params ) {
	209	+ $status = Status::newGood();
	210	+
	211	+ $ok = StreamFile::stream( $params['source'], array(), false );
	212	+ if ( !$ok ) {
	213	+ $status->fatal( "Unable to stream file {$params['source']}." );
	214	+ return $status;
	215	+ }
	216	+
	217	+ return $status;
	218	+ }
	219	+
	220	+ function getLocalCopy( array $params ) {
209	221	// Create a new temporary file...
210	222	wfSuppressWarnings();
211	223	$tmpPath = tempnam( wfTempDir(), 'file_localcopy' );
—	—	@@ -223,7 +235,7 @@
224	236
225	237	$this->chmod( $tmpPath );
226	238
227		~~- return $tmpPath;~~
	239	+ return new TempLocalFile( $tmpPath );
228	240	}
229	241
230	242	/**
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileLockManager.php
—	—	@@ -164,7 +164,7 @@
165	165	return $status;
166	166	}
167	167
168		~~- function __destruct() {~~
	168	+ protected function __destruct() {
169	169	// Make sure remaining files get cleared for sanity
170	170	foreach ( $this->handles as $key => $handler ) {
171	171	flock( $handler, LOCK_UN ); // PHP 5.3 will not do this automatically
Index: branches/FileBackend/phase3/includes/filerepo/backend/FileBackend.php
—	—	@@ -154,12 +154,25 @@
155	155	public function getFileProps( array $params );
156	156
157	157	/**
158		~~- * Get a local copy on dist of the file at a storage path in the backend~~
	158	+ * Stream the file that exists at a storage path in the backend.
	159	+ * Appropriate HTTP headers (Status, Content-Type, Content-Length)
	160	+ * must be sent on success, while no headers should be sent on failure.
	161	+ * Implementations should flush the output buffer before sending data.
159	162	* $params include:
	163	+ * source : source storage path
	164	+ *
	165	+ * @param Array $params
	166	+ * @return Status
	167	+ */
	168	+ public function streamFile( array $params );
	169	+
	170	+ /**
	171	+ * Get a local copy on disk of the file at a storage path in the backend
	172	+ * $params include:
160	173	* source : source storage path
161	174	*
162	175	* @param Array $params
163		~~- * @return string\|null Path to temporary file or null on failure~~
	176	+ * @return TempLocalFile\|null Temporary file or null on failure
164	177	*/
165	178	public function getLocalCopy( array $params );
166	179
—	—	@@ -440,7 +453,8 @@
441	454	* overwriteSame : override any existing file at destination
442	455	*/
443	456	class FileStoreOp extends FileOp {
444		~~- protected $tmpDestPath; // temp copy of existing destination file~~
	457	+ /** @var TempLocalFile\|null */
	458	+ protected $tmpDestFile; // temp copy of existing destination file
445	459
446	460	function doAttempt() {
447	461	// Create a backup copy of any file that exists at destination
—	—	@@ -485,8 +499,8 @@
486	500	if ( $this->backend->fileExists( $this->params['dest'] ) ) {
487	501	if ( $this->params['overwriteDest'] ) {
488	502	// Create a temporary backup copy...
489		~~- $this->tmpDestPath = $this->getLocalCopy( $this->params['dest'] );~~
490		~~- if ( $this->tmpDestPath === null ) {~~
	503	+ $this->tmpDestFile = $this->getLocalCopy( $this->params['dest'] );
	504	+ if ( !$this->tmpDestFile ) {
491	505	$status->fatal( "Could not backup destination file." );
492	506	return $status;
493	507	}
—	—	@@ -503,9 +517,9 @@
504	518	protected function restoreDest() {
505	519	$status = Status::newGood();
506	520	// Restore any file that was at the destination
507		~~- if ( $this->tmpDestPath !== null ) {~~
	521	+ if ( $this->tmpDestFile ) {
508	522	$params = array(
509		~~- 'source' => $this->tmpDestPath,~~
	523	+ 'source' => $this->tmpDestFile->getPath(),
510	524	'dest' => $this->params['dest']
511	525	);
512	526	$status = $this->backend->store( $params );

Status & tagging log

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