r104659 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r104658‎ \| r104659 \| r104660 >
Date:	08:55, 30 November 2011
Author:	j
Status:	reverted (Comments)
Tags:
Comment:	Use database to track uploaded chunks and concatenate at the end. follow up r93720
Modified paths:	/trunk/phase3/includes/AutoLoader.php (modified) (history) /trunk/phase3/includes/api/ApiUpload.php (modified) (history) /trunk/phase3/includes/filerepo/FSRepo.php (modified) (history) /trunk/phase3/includes/filerepo/FileRepo.php (modified) (history) /trunk/phase3/includes/installer/MysqlUpdater.php (modified) (history) /trunk/phase3/includes/upload/UploadBase.php (modified) (history) /trunk/phase3/includes/upload/UploadFromChunks.php (added) (history) /trunk/phase3/includes/upload/UploadFromFile.php (modified) (history) /trunk/phase3/includes/upload/UploadFromStash.php (modified) (history) /trunk/phase3/languages/messages/MessagesEn.php (modified) (history) /trunk/phase3/maintenance/archives/patch-uploadstash_chunk.sql (added) (history) /trunk/phase3/maintenance/tables.sql (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/archives/patch-uploadstash_chunk.sql
—	—	@@ -0,0 +1,3 @@
	2	+-- Adding us_chunk_inx field
	3	+ALTER TABLE /$wgDBprefix/uploadstash
	4	+ ADD us_chunk_inx int unsigned NULL;
Index: trunk/phase3/maintenance/tables.sql
—	—	@@ -966,6 +966,8 @@
967	967	us_timestamp varbinary(14) not null,
968	968
969	969	us_status varchar(50) not null,
	970	+
	971	+ us_chunk_inx int unsigned NULL,
970	972
971	973	-- file properties from File::getPropsFromPath. these may prove unnecessary.
972	974	--
Index: trunk/phase3/includes/upload/UploadFromStash.php
—	—	@@ -161,40 +161,4 @@
162	162	$this->unsaveUploadedFile();
163	163	return $rv;
164	164	}
165		-
166		- /**
167		~~- * Append a chunk to the temporary file.~~
168		- *
169		~~- * @param $chunk~~
170		~~- * @param $chunkSize~~
171		~~- * @param $offset~~
172		~~- * @return Status~~
173		~~- */~~
174		~~- public function appendChunk( $chunk, $chunkSize, $offset ) {~~
175		~~- //to use $this->getFileSize() here, db needs to be updated~~
176		~~- //in appendToUploadFile for that~~
177		~~- $fileSize = $this->stash->getFile( $this->mFileKey )->getSize();~~
178		~~- if ( $fileSize + $chunkSize > $this->getMaxUploadSize()) {~~
179		~~- $status = Status::newFatal( 'file-too-large' );~~
180		~~- } else {~~
181		~~- //append chunk~~
182		~~- if ( $fileSize == $offset ) {~~
183		~~- $status = $this->appendToUploadFile( $chunk,~~
184		~~- $this->mVirtualTempPath );~~
185		~~- } else {~~
186		~~- $status = Status::newFatal( 'invalid-chunk-offset' );~~
187		~~- }~~
188		~~- }~~
189		~~- return $status;~~
190		~~- }~~
191		-
192		- /**
193		~~- * Append the final chunk and ready file for parent::performUpload()~~
194		~~- * @return void~~
195		~~- */~~
196		~~- public function finalizeFile() {~~
197		~~- $this->appendFinish ( $this->mVirtualTempPath );~~
198		~~- $this->cleanupTempFile();~~
199		~~- $this->mTempPath = $this->getRealPath( $this->mVirtualTempPath );~~
200		~~- }~~
201	165	}
Index: trunk/phase3/includes/upload/UploadBase.php
—	—	@@ -199,23 +199,31 @@
200	200	/**
201	201	* Append a file to the Repo file
202	202	*
	203	+ * @deprecated since 1.19
	204	+ *
203	205	* @param $srcPath String: path to source file
204	206	* @param $toAppendPath String: path to the Repo file that will be appended to.
205	207	* @return Status Status
206	208	*/
207	209	protected function appendToUploadFile( $srcPath, $toAppendPath ) {
	210	+ wfDeprecated(__METHOD__);
	211	+
208	212	$repo = RepoGroup::singleton()->getLocalRepo();
209	213	$status = $repo->append( $srcPath, $toAppendPath );
210	214	return $status;
211	215	}
212		-
	216	+
213	217	/**
214	218	* Finish appending to the Repo file
215		- *
	219	+ *
	220	+ * @deprecated since 1.19
	221	+ *
216	222	* @param $toAppendPath String: path to the Repo file that will be appended to.
217	223	* @return Status Status
218	224	*/
219	225	protected function appendFinish( $toAppendPath ) {
	226	+ wfDeprecated(__METHOD__);
	227	+
220	228	$repo = RepoGroup::singleton()->getLocalRepo();
221	229	$status = $repo->appendFinish( $toAppendPath );
222	230	return $status;
—	—	@@ -760,7 +768,6 @@
761	769	public function stashFile() {
762	770	// was stashSessionFile
763	771	$stash = RepoGroup::singleton()->getLocalRepo()->getUploadStash();
764		-
765	772	$file = $stash->stashFile( $this->mTempPath, $this->getSourceType() );
766	773	$this->mLocalFile = $file;
767	774	return $file;
Index: trunk/phase3/includes/upload/UploadFromChunks.php
—	—	@@ -0,0 +1,253 @@
	2	+<?php
	3	+/**
	4	+ * Implements uploading from chunks
	5	+ *
	6	+ * @file
	7	+ * @ingroup upload
	8	+ * @author Michael Dale
	9	+ */
	10	+
	11	+class UploadFromChunks extends UploadFromFile {
	12	+ protected $mOffset, $mChunkIndex, $mFileKey, $mVirtualTempPath;
	13	+
	14	+ /**
	15	+ * Setup local pointers to stash, repo and user ( similar to UploadFromStash )
	16	+ *
	17	+ * @param $user User
	18	+ * @param $stash UploadStash
	19	+ * @param $repo FileRepo
	20	+ */
	21	+ public function __construct( $user = false, $stash = false, $repo = false ) {
	22	+ // user object. sometimes this won't exist, as when running from cron.
	23	+ $this->user = $user;
	24	+
	25	+ if( $repo ) {
	26	+ $this->repo = $repo;
	27	+ } else {
	28	+ $this->repo = RepoGroup::singleton()->getLocalRepo();
	29	+ }
	30	+
	31	+ if( $stash ) {
	32	+ $this->stash = $stash;
	33	+ } else {
	34	+ if( $user ) {
	35	+ wfDebug( __METHOD__ . " creating new UploadFromChunks instance for " . $user->getId() . "\n" );
	36	+ } else {
	37	+ wfDebug( __METHOD__ . " creating new UploadFromChunks instance with no user\n" );
	38	+ }
	39	+ $this->stash = new UploadStash( $this->repo, $this->user );
	40	+ }
	41	+
	42	+ return true;
	43	+ }
	44	+ /**
	45	+ * Calls the parent stashFile and updates the uploadsession table to handle "chunks"
	46	+ *
	47	+ * @return UploadStashFile stashed file
	48	+ */
	49	+ public function stashFile() {
	50	+ // Stash file is the called on creating a new chunk session:
	51	+ $this->mChunkIndex = 0;
	52	+ $this->mOffset = 0;
	53	+ // Create a local stash target
	54	+ $this->mLocalFile = parent::stashFile();
	55	+ // Update the initial file offset ( based on file size )
	56	+ $this->mOffset = $this->mLocalFile->getSize();
	57	+ $this->mFileKey = $this->mLocalFile->getFileKey();
	58	+
	59	+ // Output a copy of this first to chunk 0 location:
	60	+ $status = $this->outputChunk( $this->mLocalFile->getPath() );
	61	+
	62	+ // Update db table to reflect initial "chunk" state
	63	+ $this->updateChunkStatus();
	64	+ return $this->mLocalFile;
	65	+ }
	66	+
	67	+ /**
	68	+ * Continue chunk uploading
	69	+ */
	70	+ public function continueChunks( $name, $key, $webRequestUpload ) {
	71	+ $this->mFileKey = $key;
	72	+ $this->mUpload = $webRequestUpload;
	73	+ // Get the chunk status form the db:
	74	+ $this->getChunkStatus();
	75	+
	76	+ $metadata = $this->stash->getMetadata( $key );
	77	+ $this->initializePathInfo( $name,
	78	+ $this->getRealPath ( $metadata['us_path'] ),
	79	+ $metadata['us_size'],
	80	+ false
	81	+ );
	82	+ }
	83	+
	84	+ /**
	85	+ * Append the final chunk and ready file for parent::performUpload()
	86	+ * @return void
	87	+ */
	88	+ public function concatenateChunks() {
	89	+ wfDebug( __METHOD__ . " concatenate {$this->mChunkIndex} chunks:" .
	90	+ $this->getOffset() . ' inx:' . $this->getChunkIndex() . "\n" );
	91	+
	92	+ // Concatenate all the chunks to mVirtualTempPath
	93	+ $fileList = Array();
	94	+ // The first chunk is stored at the mVirtualTempPath path so we start on "chunk 1"
	95	+ for( $i = 0; $i <= $this->getChunkIndex(); $i++ ){
	96	+ $fileList[] = $this->getVirtualChunkLocation( $i );
	97	+ }
	98	+
	99	+ // Concatinate into the mVirtualTempPath location;
	100	+ $status = $this->repo->concatenate( $fileList, $this->mVirtualTempPath, FileRepo::DELETE_SOURCE );
	101	+ if( !$status->isOk() ){
	102	+ return $status;
	103	+ }
	104	+ // Update the mTempPath variable ( for FileUpload or normal Stash to take over )
	105	+ $this->mTempPath = $this->getRealPath( $this->mVirtualTempPath );
	106	+ return $status;
	107	+ }
	108	+ /**
	109	+ * Returns the virtual chunk location:
	110	+ * @param unknown_type $index
	111	+ */
	112	+ function getVirtualChunkLocation( $index ){
	113	+ return $this->repo->getVirtualUrl( 'temp' ) .
	114	+ '/' .
	115	+ $this->repo->getHashPath(
	116	+ $this->getChunkFileKey( $index )
	117	+ ) .
	118	+ $this->getChunkFileKey( $index );
	119	+ }
	120	+ /**
	121	+ * Add a chunk to the temporary directory
	122	+ *
	123	+ * @param $chunkPath path to temporary chunk file
	124	+ * @param $chunkSize size of the current chunk
	125	+ * @param $offset offset of current chunk ( mutch match database chunk offset )
	126	+ * @return Status
	127	+ */
	128	+ public function addChunk( $chunkPath, $chunkSize, $offset ) {
	129	+ // Get the offset before we add the chunk to the file system
	130	+ $preAppendOffset = $this->getOffset();
	131	+
	132	+ if ( $preAppendOffset + $chunkSize > $this->getMaxUploadSize()) {
	133	+ $status = Status::newFatal( 'file-too-large' );
	134	+ } else {
	135	+ // Make sure the client is uploading the correct chunk with a matching offset.
	136	+ if ( $preAppendOffset == $offset ) {
	137	+ // Update local chunk index for the current chunk
	138	+ $this->mChunkIndex++;
	139	+ $status = $this->outputChunk( $chunkPath );
	140	+ if( $status->isGood() ){
	141	+ // Update local offset:
	142	+ $this->mOffset = $preAppendOffset + $chunkSize;
	143	+ // Update chunk table status db
	144	+ $this->updateChunkStatus();
	145	+ }
	146	+ } else {
	147	+ $status = Status::newFatal( 'invalid-chunk-offset' );
	148	+ }
	149	+ }
	150	+ return $status;
	151	+ }
	152	+
	153	+ /**
	154	+ * Update the chunk db table with the current status:
	155	+ */
	156	+ private function updateChunkStatus(){
	157	+ wfDebug( __METHOD__ . " update chunk status for {$this->mFileKey} offset:" .
	158	+ $this->getOffset() . ' inx:' . $this->getChunkIndex() . "\n" );
	159	+
	160	+ $dbw = $this->repo->getMasterDb();
	161	+ $dbw->update(
	162	+ 'uploadstash',
	163	+ array(
	164	+ 'us_status' => 'chunks',
	165	+ 'us_chunk_inx' => $this->getChunkIndex(),
	166	+ 'us_size' => $this->getOffset()
	167	+ ),
	168	+ array( 'us_key' => $this->mFileKey ),
	169	+ __METHOD__
	170	+ );
	171	+ }
	172	+ /**
	173	+ * Get the chunk db state and populate update relevant local values
	174	+ */
	175	+ private function getChunkStatus(){
	176	+ $dbr = $this->repo->getSlaveDb();
	177	+ $row = $dbr->selectRow(
	178	+ 'uploadstash',
	179	+ array(
	180	+ 'us_chunk_inx',
	181	+ 'us_size',
	182	+ 'us_path',
	183	+ ),
	184	+ array( 'us_key' => $this->mFileKey ),
	185	+ __METHOD__
	186	+ );
	187	+ // Handle result:
	188	+ if ( $row ) {
	189	+ $this->mChunkIndex = $row->us_chunk_inx;
	190	+ $this->mOffset = $row->us_size;
	191	+ $this->mVirtualTempPath = $row->us_path;
	192	+ }
	193	+ }
	194	+ /**
	195	+ * Get the current Chunk index
	196	+ * @return Integer index of the current chunk
	197	+ */
	198	+ private function getChunkIndex(){
	199	+ if( $this->mChunkIndex !== null ){
	200	+ return $this->mChunkIndex;
	201	+ }
	202	+ return 0;
	203	+ }
	204	+
	205	+ /**
	206	+ * Gets the current offset in fromt the stashedupload table
	207	+ * @return Integer current byte offset of the chunk file set
	208	+ */
	209	+ private function getOffset(){
	210	+ if ( $this->mOffset !== null ){
	211	+ return $this->mOffset;
	212	+ }
	213	+ return 0;
	214	+ }
	215	+
	216	+ /**
	217	+ * Output the chunk to disk
	218	+ *
	219	+ * @param $chunk
	220	+ * @param unknown_type $path
	221	+ */
	222	+ private function outputChunk( $chunkPath ){
	223	+ // Key is fileKey + chunk index
	224	+ $fileKey = $this->getChunkFileKey();
	225	+
	226	+ // Store the chunk per its indexed fileKey:
	227	+ $hashPath = $this->repo->getHashPath( $fileKey );
	228	+ $storeStatus = $this->repo->store( $chunkPath, 'temp', "$hashPath$fileKey" );
	229	+
	230	+ // Check for error in stashing the chunk:
	231	+ if ( ! $storeStatus->isOK() ) {
	232	+ $error = $storeStatus->getErrorsArray();
	233	+ $error = reset( $error );
	234	+ if ( ! count( $error ) ) {
	235	+ $error = $storeStatus->getWarningsArray();
	236	+ $error = reset( $error );
	237	+ if ( ! count( $error ) ) {
	238	+ $error = array( 'unknown', 'no error recorded' );
	239	+ }
	240	+ }
	241	+ throw new UploadChunkFileException( "error storing file in '$path': " . implode( '; ', $error ) );
	242	+ }
	243	+ return $storeStatus;
	244	+ }
	245	+ private function getChunkFileKey( $index = null ){
	246	+ if( $index === null ){
	247	+ $index = $this->getChunkIndex();
	248	+ }
	249	+ return $this->mFileKey . '.' . $index ;
	250	+ }
	251	+}
	252	+
	253	+class UploadChunkZeroLengthFileException extends MWException {};
	254	+class UploadChunkFileException extends MWException {};
Index: trunk/phase3/includes/upload/UploadFromFile.php
—	—	@@ -75,12 +75,4 @@
76	76
77	77	return parent::verifyUpload();
78	78	}
79		-
80		- /**
81		~~- * Get the path to the file underlying the upload~~
82		~~- * @return String path to file~~
83		~~- */~~
84		~~- public function getFileTempname() {~~
85		~~- return $this->mUpload->getTempname();~~
86		~~- }~~
87	79	}
Index: trunk/phase3/includes/filerepo/FSRepo.php
—	—	@@ -313,11 +313,63 @@
314	314	wfRestoreWarnings();
315	315	}
316	316	}
317		-
318	317	/**
	318	+ * Concatenate a list of files into a target file location.
	319	+ *
	320	+ * @param $fileList array of files
	321	+ * @param $targetFile String target path
	322	+ * @param $flags Integer: bitwise combination of the following flags:
	323	+ * self::FILES_ONLY Mark file as existing only if it is a file (not directory)
	324	+ */
	325	+ function concatenate( $fileList, $targetPath, $flags = 0 ){
	326	+ $status = $this->newGood();
	327	+ // Resolve the virtual URL for taget:
	328	+ if ( self::isVirtualUrl( $targetPath ) ) {
	329	+ $targetPath = $this->resolveVirtualUrl( $targetPath );
	330	+ // empty out the target file:
	331	+ if ( is_file( $targetPath ) ){
	332	+ unlink( $targetPath );
	333	+ }
	334	+ }
	335	+ foreach( $fileList as $sourcePath ){
	336	+ // Resolve the virtual URL for source:
	337	+ if ( self::isVirtualUrl( $sourcePath ) ) {
	338	+ $sourcePath = $this->resolveVirtualUrl( $sourcePath );
	339	+ }
	340	+ if ( !is_file( $sourcePath ) )
	341	+ $status->fatal( 'filenotfound', $sourcePath );
	342	+
	343	+ if ( !$status->isOk() ){
	344	+ return $status;
	345	+ }
	346	+
	347	+ // Do the append
	348	+ $chunk = file_get_contents( $sourcePath );
	349	+ if( $chunk === false ) {
	350	+ $status->fatal( 'fileconcatenateerrorread', $sourcePath );
	351	+ return $status;
	352	+ }
	353	+ if( $status->isOk() ) {
	354	+ if ( file_put_contents( $targetPath, $chunk, FILE_APPEND ) ) {
	355	+ $status->value = $targetPath;
	356	+ } else {
	357	+ $status->fatal( 'fileconcatenateerror', $sourcePath, $targetPath);
	358	+ }
	359	+ }
	360	+ if ( $flags & self::DELETE_SOURCE ) {
	361	+ unlink( $sourcePath );
	362	+ }
	363	+ }
	364	+ return $status;
	365	+ }
	366	+ /**
	367	+ * @deprecated 1.19
	368	+ *
319	369	* @return Status
320	370	*/
321	371	function append( $srcPath, $toAppendPath, $flags = 0 ) {
	372	+ wfDeprecated(__METHOD__);
	373	+
322	374	$status = $this->newGood();
323	375
324	376	// Resolve the virtual URL
Index: trunk/phase3/includes/filerepo/FileRepo.php
—	—	@@ -421,6 +421,14 @@
422	422
423	423
424	424	/**
	425	+ * Concatenate and array of file sources.
	426	+ * @param $fileList Array of file sources
	427	+ * @param $targetPath String target destination for file.
	428	+ * @throws MWException
	429	+ */
	430	+ abstract function concatenate( $fileList, $targetPath, $flags = 0 );
	431	+
	432	+ /**
425	433	* Append the contents of the source path to the given file, OR queue
426	434	* the appending operation in anticipation of a later appendFinish() call.
427	435	* @param $srcPath String: location of the source file
Index: trunk/phase3/includes/installer/MysqlUpdater.php
—	—	@@ -190,6 +190,7 @@
191	191	array( 'addField', 'archive', 'ar_sha1', 'patch-ar_sha1.sql' ),
192	192	array( 'addIndex', 'page', 'page_redirect_namespace_len', 'patch-page_redirect_namespace_len.sql' ),
193	193	array( 'modifyField', 'user', 'ug_group', 'patch-ug_group-length-increase.sql' ),
	194	+ array( 'addField', 'uploadstash', 'us_chunk_inx', 'patch-uploadstash_chunk.sql' ),
194	195	);
195	196	}
196	197
Index: trunk/phase3/includes/api/ApiUpload.php
—	—	@@ -89,8 +89,7 @@
90	90	} else {
91	91	$this->verifyUpload();
92	92	}
93		-
94		-
	93	+
95	94	// Check if the user has the rights to modify or overwrite the requested title
96	95	// (This check is irrelevant if stashing is already requested, since the errors
97	96	// can always be fixed by changing the title)
—	—	@@ -100,68 +99,105 @@
101	100	$this->dieRecoverableError( $permErrors[0], 'filename' );
102	101	}
103	102	}
	103	+ // Get the result based on the current upload context:
	104	+ $result = $this->getContextResult();
104	105
105		~~- // Prepare the API result~~
106		~~- $result = array();~~
	106	+ if ( $result['result'] === 'Success' ) {
	107	+ $result['imageinfo'] = $this->mUpload->getImageInfo( $this->getResult() );
	108	+ }
107	109
	110	+ $this->getResult()->addValue( null, $this->getModuleName(), $result );
	111	+
	112	+ // Cleanup any temporary mess
	113	+ $this->mUpload->cleanupTempFile();
	114	+ }
	115	+ /**
	116	+ * Get an uplaod result based on upload context
	117	+ */
	118	+ private function getContextResult(){
108	119	$warnings = $this->getApiWarnings();
109	120	if ( $warnings ) {
110		~~- $result['result'] = 'Warning';~~
111		~~- $result['warnings'] = $warnings;~~
112		~~- // in case the warnings can be fixed with some further user action, let's stash this upload~~
113		~~- // and return a key they can use to restart it~~
114		~~- try {~~
115		~~- $result['filekey'] = $this->performStash();~~
116		~~- $result['sessionkey'] = $result['filekey']; // backwards compatibility~~
117		~~- } catch ( MWException $e ) {~~
118		~~- $result['warnings']['stashfailed'] = $e->getMessage();~~
	121	+ // Get warnings formated in result array format
	122	+ return $this->getWarningsResult( $warnings );
	123	+ } elseif ( $this->mParams['chunk'] ) {
	124	+ // Add chunk, and get result
	125	+ return $this->getChunkResult();
	126	+ } elseif ( $this->mParams['stash'] ) {
	127	+ // Stash the file and get stash result
	128	+ return $this->getStashResult();
	129	+ }
	130	+ // This is the most common case -- a normal upload with no warnings
	131	+ // performUpload will return a formatted properly for the API with status
	132	+ return $this->performUpload();
	133	+ }
	134	+ /**
	135	+ * Get Stash Result, throws an expetion if the file could not be stashed.
	136	+ */
	137	+ private function getStashResult(){
	138	+ $result = array ();
	139	+ // Some uploads can request they be stashed, so as not to publish them immediately.
	140	+ // In this case, a failure to stash ought to be fatal
	141	+ try {
	142	+ $result['result'] = 'Success';
	143	+ $result['filekey'] = $this->performStash();
	144	+ $result['sessionkey'] = $result['filekey']; // backwards compatibility
	145	+ } catch ( MWException $e ) {
	146	+ $this->dieUsage( $e->getMessage(), 'stashfailed' );
	147	+ }
	148	+ return $result;
	149	+ }
	150	+ /**
	151	+ * Get Warnings Result
	152	+ * @param $warnings Array of Api upload warnings
	153	+ */
	154	+ private function getWarningsResult( $warnings ){
	155	+ $result = array();
	156	+ $result['result'] = 'Warning';
	157	+ $result['warnings'] = $warnings;
	158	+ // in case the warnings can be fixed with some further user action, let's stash this upload
	159	+ // and return a key they can use to restart it
	160	+ try {
	161	+ $result['filekey'] = $this->performStash();
	162	+ $result['sessionkey'] = $result['filekey']; // backwards compatibility
	163	+ } catch ( MWException $e ) {
	164	+ $result['warnings']['stashfailed'] = $e->getMessage();
	165	+ }
	166	+ return $result;
	167	+ }
	168	+ /**
	169	+ * Get the result of a chunk upload.
	170	+ */
	171	+ private function getChunkResult(){
	172	+ $result = array();
	173	+
	174	+ $result['result'] = 'Continue';
	175	+ $request = $this->getMain()->getRequest();
	176	+ $chunkPath = $request->getFileTempname( 'chunk' );
	177	+ $chunkSize = $request->getUpload( 'chunk' )->getSize();
	178	+ if ($this->mParams['offset'] == 0) {
	179	+ $result['filekey'] = $this->performStash();
	180	+ } else {
	181	+ $status = $this->mUpload->addChunk($chunkPath, $chunkSize,
	182	+ $this->mParams['offset']);
	183	+ if ( !$status->isGood() ) {
	184	+ $this->dieUsage( $status->getWikiText(), 'stashfailed' );
	185	+ return ;
119	186	}
120		~~- } elseif ( $this->mParams['chunk'] ) {~~
121		~~- $result['result'] = 'Continue';~~
122		~~- $chunk = $request->getFileTempName( 'chunk' );~~
123		~~- $chunkSize = $request->getUpload( 'chunk' )->getSize();~~
124		~~- if ($this->mParams['offset'] == 0) {~~
125		~~- $result['filekey'] = $this->performStash();~~
126		~~- } else {~~
127		~~- $status = $this->mUpload->appendChunk($chunk, $chunkSize,~~
128		~~- $this->mParams['offset']);~~
	187	+ $result['filekey'] = $this->mParams['filekey'];
	188	+ // Check we added the last chunk:
	189	+ if( $this->mParams['offset'] + $chunkSize == $this->mParams['filesize'] ) {
	190	+ $status = $this->mUpload->concatenateChunks();
129	191	if ( !$status->isGood() ) {
130	192	$this->dieUsage( $status->getWikiText(), 'stashfailed' );
131		~~- } else {~~
132		~~- $result['filekey'] = $this->mParams['filekey'];~~
133		~~- if($this->mParams['offset'] + $chunkSize == $this->mParams['filesize']) {~~
134		~~- $this->mUpload->finalizeFile();~~
135		~~- $result['result'] = 'Success';~~
136		~~- }~~
	193	+ return ;
137	194	}
138		~~- }~~
139		~~- $result['offset'] = $this->mParams['offset'] + $chunkSize;~~
140		~~- } elseif ( $this->mParams['stash'] ) {~~
141		~~- // Some uploads can request they be stashed, so as not to publish them immediately.~~
142		~~- // In this case, a failure to stash ought to be fatal~~
143		~~- try {~~
144	195	$result['result'] = 'Success';
145		~~- $result['filekey'] = $this->performStash();~~
146		~~- $result['sessionkey'] = $result['filekey']; // backwards compatibility~~
147		~~- } catch ( MWException $e ) {~~
148		~~- $this->dieUsage( $e->getMessage(), 'stashfailed' );~~
149	196	}
150		~~- } else {~~
151		~~- // This is the most common case -- a normal upload with no warnings~~
152		~~- // $result will be formatted properly for the API already, with a status~~
153		~~- $result = $this->performUpload();~~
154	197	}
155		-
156		~~- if ( $result['result'] === 'Success' ) {~~
157		~~- $result['imageinfo'] = $this->mUpload->getImageInfo( $this->getResult() );~~
158		~~- }~~
159		-
160		~~- $this->getResult()->addValue( null, $this->getModuleName(), $result );~~
161		-
162		~~- // Cleanup any temporary mess~~
163		~~- $this->mUpload->cleanupTempFile();~~
	198	+ $result['offset'] = $this->mParams['offset'] + $chunkSize;
	199	+ return $result;
164	200	}
165		-
	201	+
166	202	/**
167	203	* Stash the file and return the file key
168	204	* Also re-raises exceptions with slightly more informative message strings (useful for API)
—	—	@@ -244,7 +280,24 @@
245	281	$this->dieUsageMsg( array( 'missingparam', 'filename' ) );
246	282	}
247	283
248		~~- if ( $this->mParams['filekey'] ) {~~
	284	+ if ( $this->mParams['chunk'] ) {
	285	+ // Chunk upload
	286	+ $this->mUpload = new UploadFromChunks();
	287	+ if( isset( $this->mParams['filekey'] ) ){
	288	+ // handle new chunk
	289	+ $this->mUpload->continueChunks(
	290	+ $this->mParams['filename'],
	291	+ $this->mParams['filekey'],
	292	+ $request->getUpload( 'chunk' )
	293	+ );
	294	+ } else {
	295	+ // handle first chunk
	296	+ $this->mUpload->initialize(
	297	+ $this->mParams['filename'],
	298	+ $request->getUpload( 'chunk' )
	299	+ );
	300	+ }
	301	+ } elseif ( isset( $this->mParams['filekey'] ) ) {
249	302	// Upload stashed in a previous request
250	303	if ( !UploadFromStash::isValidKey( $this->mParams['filekey'] ) ) {
251	304	$this->dieUsageMsg( 'invalid-file-key' );
—	—	@@ -253,14 +306,6 @@
254	307	$this->mUpload = new UploadFromStash( $this->getUser() );
255	308
256	309	$this->mUpload->initialize( $this->mParams['filekey'], $this->mParams['filename'] );
257		-
258		~~- } elseif ( isset( $this->mParams['chunk'] ) ) {~~
259		~~- // Start new Chunk upload~~
260		~~- $this->mUpload = new UploadFromFile();~~
261		~~- $this->mUpload->initialize(~~
262		~~- $this->mParams['filename'],~~
263		~~- $request->getUpload( 'chunk' )~~
264		~~- );~~
265	310	} elseif ( isset( $this->mParams['file'] ) ) {
266	311	$this->mUpload = new UploadFromFile();
267	312	$this->mUpload->initialize(
Index: trunk/phase3/includes/AutoLoader.php
—	—	@@ -839,6 +839,7 @@
840	840	# includes/upload
841	841	'UploadBase' => 'includes/upload/UploadBase.php',
842	842	'UploadFromFile' => 'includes/upload/UploadFromFile.php',
	843	+ 'UploadFromChunks' => 'includes/upload/UploadFromChunks.php',
843	844	'UploadFromStash' => 'includes/upload/UploadFromStash.php',
844	845	'UploadFromUrl' => 'includes/upload/UploadFromUrl.php',
845	846	'UploadStash' => 'includes/upload/UploadStash.php',
Index: trunk/phase3/languages/messages/MessagesEn.php
—	—	@@ -2252,6 +2252,7 @@
2253	2253	'uploadstash-badtoken' => 'Performing of that action was unsuccessful, perhaps because your editing credentials expired. Try again.',
2254	2254	'uploadstash-errclear' => 'Clearing the files was unsuccessful.',
2255	2255	'uploadstash-refresh' => 'Refresh the list of files',
	2256	+'invalid-chunk-offset' => 'Invalid chunk offset',
2256	2257
2257	2258	# img_auth script messages
2258	2259	'img-auth-accessdenied' => 'Access denied',

Follow-up revisions

Revision	Commit summary	Author	Date
r104665	add i18n#Message_documentation for the newly added messages and add entry to ...	j	12:13, 30 November 2011
r104680	Revert r104659 and its followup r104665: break the unit tests with a fatal er...	catrope	13:54, 30 November 2011

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r93720	Extend upload api adding an option to upload files in chunks,...	j	10:13, 2 August 2011

Comments

#Comment by Siebrand (talk | contribs) 11:51, 30 November 2011

Please add message documentation for the newly added messages. Thanks.
entry in messages.inc missing also see i18n

#Comment by Catrope (talk | contribs) 13:47, 30 November 2011

Breaks the unit tests:

 [exec] PHP Fatal error:  Class NullRepo contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (FileRepo::concatenate) in /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/includes/filerepo/NullRepo.php on line 47

#Comment by Hashar (talk | contribs) 13:49, 30 November 2011

Breaks Jenkins build system:

[exec] PHP Fatal error:  Class NullRepo contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (FileRepo::concatenate) in /var/lib/jenkins/jobs/MediaWiki-phpunit/workspace/mw-core/includes/filerepo/NullRepo.php on line 47

See console output at http://integration.mediawiki.org/ci/job/MediaWiki-phpunit/3041/console

You need to run the tests before sending a commit :-) Example:

$ cd tests/phpunit
$ php phpunit.php

That would catch the issue.

#Comment by Hashar (talk | contribs) 13:50, 30 November 2011

Backtrace as of r104659 :

Fatal error: Class NullRepo contains 1 abstract method and must therefore be declared abstract
or implement the remaining methods (FileRepo::concatenate) in /srv/trunk/includes/filerepo/NullRepo.php on line 47
Call Stack:
   0.0009     670152   1. {main}() /srv/trunk/tests/phpunit/phpunit.php:0
   0.0734   13528800   2. MediaWikiPHPUnitCommand::main() /srv/trunk/tests/phpunit/phpunit.php:60
   0.0734   13544016   3. PHPUnit_TextUI_Command->run() /srv/trunk/tests/phpunit/MediaWikiPHPUnitCommand.php:44
   1.2213   75047024   4. PHPUnit_TextUI_TestRunner->doRun() /opt/local/share/pear/PHPUnit/TextUI/Command.php:187
   1.2337   75601488   5. PHPUnit_Framework_TestSuite->run() /opt/local/share/pear/PHPUnit/TextUI/TestRunner.php:325
   1.2338   75602976   6. PHPUnit_Framework_TestSuite->run() /opt/local/share/pear/PHPUnit/Framework/TestSuite.php:705
  23.5629  153762784   7. PHPUnit_Framework_TestSuite->run() /opt/local/share/pear/PHPUnit/Framework/TestSuite.php:705
  23.5630  153763800   8. PHPUnit_Framework_TestSuite->run() /opt/local/share/pear/PHPUnit/Framework/TestSuite.php:705
  23.5630  153765256   9. PHPUnit_Framework_TestSuite->runTest() /opt/local/share/pear/PHPUnit/Framework/TestSuite.php:745
  23.5630  153765256  10. MediaWikiTestCase->run() /opt/local/share/pear/PHPUnit/Framework/TestSuite.php:772
  23.5630  153765256  11. PHPUnit_Framework_TestCase->run() /srv/trunk/tests/phpunit/MediaWikiTestCase.php:68
  23.5631  153765256  12. PHPUnit_Framework_TestResult->run() /opt/local/share/pear/PHPUnit/Framework/TestCase.php:727
  23.5631  153766872  13. PHPUnit_Framework_TestCase->runBare() /opt/local/share/pear/PHPUnit/Framework/TestResult.php:649
  23.5632  153808544  14. PHPUnit_Framework_TestCase->runTest() /opt/local/share/pear/PHPUnit/Framework/TestCase.php:780
  23.5632  153811112  15. ReflectionMethod->invokeArgs() /opt/local/share/pear/PHPUnit/Framework/TestCase.php:918
  23.5632  153811192  16. BitmapScalingTest->testNormaliseParams() /opt/local/share/pear/PHPUnit/Framework/TestCase.php:918
  23.5632  153814048  17. FakeDimensionFile->__construct() /srv/trunk/tests/phpunit/includes/media/BitmapScalingTest.php:21
  23.5633  153819616  18. AutoLoader::autoload() /srv/trunk/tests/phpunit/includes/media/BitmapScalingTest.php:0

#Comment by Catrope (talk | contribs) 13:56, 30 November 2011

I've reverted this (and r104665) so the unit tests will work again. Feel free to reapply once you've confirmed that the unit tests suite doesn't crap out with a fatal error.

If it was just test failures, it wouldn't have been that bad, but a fatal error means the tests don't complete and test failures can sneak in without anyone noticing.

Status & tagging log

13:55, 30 November 2011 Catrope (talk | contribs) changed the status of r104659 [removed: fixme added: reverted]
11:51, 30 November 2011 Siebrand (talk | contribs) changed the status of r104659 [removed: new added: fixme]