r72475 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r72474‎ \| r72475 \| r72476 >
Date:	10:18, 6 September 2010
Author:	btongminh
Status:	resolved (Comments)
Tags:
Comment:	Follow-up r70137: Made asynchronous upload working a bit more. It now fully works from the API; works still needs to be done for the normal UI. PHPUnit tests are updated and should cover most code paths that can be called from the API. ApiUpload: * Added "statuskey" parameter; this is the key that is returned by an async upload * Refactored warnings transformation into its own function * filename is no longer required on all uploads UploadFromUrlJob: * Moved upload results to its own entry in $_SESSION, instead of using the one from upload * Fix storing in session by calling wfSetupSession and session_write_close where needed Tests: * Set $wgUser in ApiSetup, so that individual tests don't have to do this for themselves * Added tests to cover most code paths from the API * Fixed UploadFromUrlTestSuite so that its tests are included in a regular phpunit invocation (something strange with the AutoLoader; not sure what) Other files: * Allow passing session id to wfSetupSession * Explicitly close the session before doing jobs, so that jobs can't manipulate the current session
Modified paths:	/trunk/phase3/RELEASE-NOTES (modified) (history) /trunk/phase3/includes/GlobalFunctions.php (modified) (history) /trunk/phase3/includes/Wiki.php (modified) (history) /trunk/phase3/includes/api/ApiUpload.php (modified) (history) /trunk/phase3/includes/job/UploadFromUrlJob.php (modified) (history) /trunk/phase3/includes/upload/UploadFromUrl.php (modified) (history) /trunk/phase3/maintenance/tests/ApiSetup.php (modified) (history) /trunk/phase3/maintenance/tests/UploadFromUrlTest.php (modified) (history) /trunk/phase3/maintenance/tests/UploadFromUrlTestSuite.php (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/tests/UploadFromUrlTestSuite.php
—	—	@@ -170,6 +170,10 @@
171	171	}
172	172
173	173	public static function suite() {
174		~~- return new UploadFromUrlTestSuite( 'UploadFromUrlTest' );~~
	174	+ // Hack to invoke the autoloader required to get phpunit to recognize
	175	+ // the UploadFromUrlTest class
	176	+ class_exists( 'UploadFromUrlTest' );
	177	+ $suite = new UploadFromUrlTestSuite( 'UploadFromUrlTest' );
	178	+ return $suite;
175	179	}
176	180	}
Index: trunk/phase3/maintenance/tests/ApiSetup.php
—	—	@@ -22,7 +22,7 @@
23	23	static function setupUser() {
24	24	if ( self::$user == NULL ) {
25	25	self::$userName = "Useruser";
26		~~- self::$passWord = User::randomPassword();~~
	26	+ self::$passWord = 'Passpass';
27	27
28	28	self::$user = User::newFromName( self::$userName );
29	29	if ( !self::$user->getID() ) {
—	—	@@ -33,5 +33,7 @@
34	34	self::$user->setPassword( self::$passWord );
35	35	self::$user->saveSettings();
36	36	}
	37	+
	38	+ $GLOBALS['wgUser'] = self::$user;
37	39	}
38	40	}
Index: trunk/phase3/maintenance/tests/UploadFromUrlTest.php
—	—	@@ -1,44 +1,50 @@
2	2	<?php
3	3
4		~~-/* Force User.php include for EDIT_TOKEN_SUFFIX */~~
5		~~-require_once dirname(__FILE__) . "/../../includes/User.php";~~
6	4
7		~~-class nullClass {~~
8		~~- public function handleOutput() { }~~
9		~~- public function purgeRedundantText() { }~~
10		-}
11		-
12	5	class UploadFromUrlTest extends ApiTestSetup {
13	6
14		~~- function setUp() {~~
	7	+ public function setUp() {
15	8	global $wgEnableUploads, $wgAllowCopyUploads;
	9	+ parent::setup();
16	10
17	11	$wgEnableUploads = true;
18	12	$wgAllowCopyUploads = true;
19		~~- parent::setup();~~
	13	+ wfSetupSession();
20	14
21	15	ini_set( 'log_errors', 1 );
22	16	ini_set( 'error_reporting', 1 );
23	17	ini_set( 'display_errors', 1 );
	18	+
	19	+ if ( wfLocalFile( 'UploadFromUrlTest.png' )->exists() ) {
	20	+ $this->deleteFile( 'UploadFromUrlTest.png' );
	21	+ }
24	22	}
25	23
26		~~- function doApiRequest( $params, $data = null ) {~~
27		~~- $session = isset( $data[2] ) ? $data[2] : array();~~
28		~~- $_SESSION = $session;~~
29		-
30		~~- $req = new FauxRequest( $params, true, $session );~~
	24	+ protected function doApiRequest( $params ) {
	25	+ $sessionId = session_id();
	26	+ session_write_close();
	27	+
	28	+ $req = new FauxRequest( $params, true, $_SESSION );
31	29	$module = new ApiMain( $req, true );
32	30	$module->execute();
33	31
34		~~- return array( $module->getResultData(), $req, $_SESSION );~~
	32	+ wfSetupSession( $sessionId );
	33	+ return array( $module->getResultData(), $req );
35	34	}
36	35
37		~~- function testClearQueue() {~~
	36	+ /**
	37	+ * Ensure that the job queue is empty before continuing
	38	+ */
	39	+ public function testClearQueue() {
38	40	while ( $job = Job::pop() ) { }
39	41	$this->assertFalse( $job );
40	42	}
41	43
42		~~- function testLogin() {~~
	44	+ /**
	45	+ * @todo Document why we test login, since the $wgUser hack used doesn't
	46	+ * require login
	47	+ */
	48	+ public function testLogin() {
43	49	$data = $this->doApiRequest( array(
44	50	'action' => 'login',
45	51	'lgname' => self::$userName,
—	—	@@ -64,19 +70,16 @@
65	71
66	72	/**
67	73	* @depends testLogin
	74	+ * @depends testClearQueue
68	75	*/
69		~~- function testSetupUrlDownload( $data ) {~~
70		~~- global $wgUser;~~
71		~~- $wgUser = User::newFromName( self::$userName );~~
72		~~- $wgUser->load();~~
73		~~- $data[2]['wsEditToken'] = $data[2]['wsToken'];~~
74		~~- $token = md5( $data[2]['wsToken'] ) . EDIT_TOKEN_SUFFIX;~~
	76	+ public function testSetupUrlDownload( $data ) {
	77	+ $token = self::$user->editToken();
75	78	$exception = false;
76	79
77	80	try {
78	81	$this->doApiRequest( array(
79	82	'action' => 'upload',
80		~~- ), $data );~~
	83	+ ) );
81	84	} catch ( UsageException $e ) {
82	85	$exception = true;
83	86	$this->assertEquals( "The token parameter must be set", $e->getMessage() );
—	—	@@ -91,7 +94,7 @@
92	95	), $data );
93	96	} catch ( UsageException $e ) {
94	97	$exception = true;
95		~~- $this->assertEquals( "One of the parameters sessionkey, file, url is required",~~
	98	+ $this->assertEquals( "One of the parameters sessionkey, file, url, statuskey is required",
96	99	$e->getMessage() );
97	100	}
98	101	$this->assertTrue( $exception, "Got exception" );
—	—	@@ -109,7 +112,7 @@
110	113	}
111	114	$this->assertTrue( $exception, "Got exception" );
112	115
113		~~- $wgUser->removeGroup( 'sysop' );~~
	116	+ self::$user->removeGroup( 'sysop' );
114	117	$exception = false;
115	118	try {
116	119	$this->doApiRequest( array(
—	—	@@ -124,8 +127,8 @@
125	128	}
126	129	$this->assertTrue( $exception, "Got exception" );
127	130
128		~~- $wgUser->addGroup( '*' );~~
129		~~- $wgUser->addGroup( 'sysop' );~~
	131	+ self::$user->addGroup( '*' );
	132	+ self::$user->addGroup( 'sysop' );
130	133	$exception = false;
131	134	$data = $this->doApiRequest( array(
132	135	'action' => 'upload',
—	—	@@ -143,45 +146,65 @@
144	147
145	148	/**
146	149	* @depends testLogin
	150	+ * @depends testClearQueue
147	151	*/
148		~~- function testDoDownload( $data ) {~~
149		~~- global $wgUser;~~
150		~~- $data[2]['wsEditToken'] = $data[2]['wsToken'];~~
151		~~- $token = md5( $data[2]['wsToken'] ) . EDIT_TOKEN_SUFFIX;~~
	152	+ public function testAsyncUpload( $data ) {
	153	+ $token = self::$user->editToken();
152	154
153		~~- $wgUser->addGroup( 'users' );~~
154		~~- $data = $this->doApiRequest( array(~~
	155	+ self::$user->addGroup( 'users' );
	156	+
	157	+ $data = $this->doAsyncUpload( $token, true );
	158	+ $this->assertEquals( $data[0]['upload']['result'], 'Success' );
	159	+ $this->assertEquals( $data[0]['upload']['filename'], 'UploadFromUrlTest.png' );
	160	+ $this->assertTrue( wfLocalFile( $data[0]['upload']['filename'] )->exists() );
	161	+
	162	+ $this->deleteFile( 'UploadFromUrlTest.png' );
	163	+
	164	+ return $data;
	165	+ }
	166	+
	167	+ /**
	168	+ * @depends testLogin
	169	+ * @depends testClearQueue
	170	+ */
	171	+ public function testAsyncUploadWarning( $data ) {
	172	+ $token = self::$user->editToken();
	173	+
	174	+ self::$user->addGroup( 'users' );
	175	+
	176	+
	177	+ $data = $this->doAsyncUpload( $token );
	178	+
	179	+ $this->assertEquals( $data[0]['upload']['result'], 'Warning' );
	180	+ $this->assertTrue( isset( $data[0]['upload']['sessionkey'] ) );
	181	+
	182	+ $data = $this->doApiRequest( array(
155	183	'action' => 'upload',
	184	+ 'sessionkey' => $data[0]['upload']['sessionkey'],
156	185	'filename' => 'UploadFromUrlTest.png',
157		~~- 'url' => 'http://bits.wikimedia.org/skins-1.5/common/images/poweredby_mediawiki_88x31.png',~~
158		~~- 'asyncdownload' => 1,~~
	186	+ 'ignorewarnings' => 1,
159	187	'token' => $token,
160		~~- ), $data );~~
	188	+ ) );
	189	+ $this->assertEquals( $data[0]['upload']['result'], 'Success' );
	190	+ $this->assertEquals( $data[0]['upload']['filename'], 'UploadFromUrlTest.png' );
	191	+ $this->assertTrue( wfLocalFile( $data[0]['upload']['filename'] )->exists() );
	192	+
	193	+ $this->deleteFile( 'UploadFromUrlTest.png' );
161	194
162		~~- $job = Job::pop();~~
163		~~- $this->assertEquals( 'UploadFromUrlJob', get_class( $job ) );~~
164		-
165		~~- $status = $job->run();~~
166		-
167		~~- $this->assertTrue( $status );~~
168		-
169	195	return $data;
170	196	}
171	197
172	198	/**
173	199	* @depends testLogin
	200	+ * @depends testClearQueue
174	201	*/
175		~~- function testSyncDownload( $data ) {~~
176		~~- global $wgUser;~~
177		~~- $data[2]['wsEditToken'] = $data[2]['wsToken'];~~
178		~~- $token = md5( $data[2]['wsToken'] ) . EDIT_TOKEN_SUFFIX;~~
	202	+ public function testSyncDownload( $data ) {
	203	+ $token = self::$user->editToken();
179	204
180	205	$job = Job::pop();
181	206	$this->assertFalse( $job, 'Starting with an empty jobqueue' );
182	207
183		~~- //$this->deleteFile( 'UploadFromUrlTest.png' );~~
184		-
185		~~- $wgUser->addGroup( 'users' );~~
	208	+ self::$user->addGroup( 'users' );
186	209	$data = $this->doApiRequest( array(
187	210	'action' => 'upload',
188	211	'filename' => 'UploadFromUrlTest.png',
—	—	@@ -194,25 +217,121 @@
195	218	$this->assertFalse( $job );
196	219
197	220	$this->assertEquals( 'Success', $data[0]['upload']['result'] );
	221	+ $this->deleteFile( 'UploadFromUrlTest.png' );
198	222
199	223	return $data;
200	224	}
	225	+
	226	+ public function testLeaveMessage() {
	227	+ $token = self::$user->editToken();
	228	+
	229	+ $talk = self::$user->getTalkPage();
	230	+ if ( $talk->exists() ) {
	231	+ $a = new Article( $talk );
	232	+ $a->doDeleteArticle( '' );
	233	+ }
	234	+
	235	+ $this->assertFalse( (bool)$talk->getArticleId( GAID_FOR_UPDATE ), 'User talk does not exist' );
	236	+
	237	+ $data = $this->doApiRequest( array(
	238	+ 'action' => 'upload',
	239	+ 'filename' => 'UploadFromUrlTest.png',
	240	+ 'url' => 'http://bits.wikimedia.org/skins-1.5/common/images/poweredby_mediawiki_88x31.png',
	241	+ 'asyncdownload' => 1,
	242	+ 'token' => $token,
	243	+ 'leavemessage' => 1,
	244	+ 'ignorewarnings' => 1,
	245	+ ) );
	246	+
	247	+ $job = Job::pop();
	248	+ $job->run();
	249	+
	250	+ $this->assertTrue( wfLocalFile( 'UploadFromUrlTest.png' )->exists() );
	251	+ $this->assertTrue( (bool)$talk->getArticleId( GAID_FOR_UPDATE ), 'User talk exists' );
	252	+
	253	+ $this->deleteFile( 'UploadFromUrlTest.png' );
	254	+
	255	+ $talkRev = Revision::newFromTitle( $talk );
	256	+ $talkSize = $talkRev->getSize();
	257	+
	258	+ $exception = false;
	259	+ try {
	260	+ $data = $this->doApiRequest( array(
	261	+ 'action' => 'upload',
	262	+ 'filename' => 'UploadFromUrlTest.png',
	263	+ 'url' => 'http://bits.wikimedia.org/skins-1.5/common/images/poweredby_mediawiki_88x31.png',
	264	+ 'asyncdownload' => 1,
	265	+ 'token' => $token,
	266	+ 'leavemessage' => 1,
	267	+ ) );
	268	+ } catch ( UsageException $e ) {
	269	+ $exception = true;
	270	+ $this->assertEquals( 'Using leavemessage without ignorewarnings is not supported', $e->getMessage() );
	271	+ }
	272	+ $this->assertTrue( $exception );
	273	+
	274	+ $job = Job::pop();
	275	+ $this->assertFalse( $job );
	276	+
	277	+ return;
	278	+
	279	+ // Broken until using leavemessage with ignorewarnings is supported
	280	+ $job->run();
	281	+
	282	+ $this->assertFalse( wfLocalFile( 'UploadFromUrlTest.png' )->exists() );
	283	+
	284	+ $talkRev = Revision::newFromTitle( $talk );
	285	+ $this->assertTrue( $talkRev->getSize() > $talkSize, 'New message left' );
201	286
	287	+
	288	+ }
	289	+
202	290	/**
203		~~- * @depends testDoDownload~~
	291	+ * Helper function to perform an async upload, execute the job and fetch
	292	+ * the status
	293	+ *
	294	+ * @return array The result of action=upload&statuskey=key
204	295	*/
205		~~- function testVerifyDownload( $data ) {~~
206		~~- $t = Title::newFromText( "UploadFromUrlTest.png", NS_FILE );~~
	296	+ private function doAsyncUpload( $token, $ignoreWarnings = false, $leaveMessage = false ) {
	297	+ $params = array(
	298	+ 'action' => 'upload',
	299	+ 'filename' => 'UploadFromUrlTest.png',
	300	+ 'url' => 'http://bits.wikimedia.org/skins-1.5/common/images/poweredby_mediawiki_88x31.png',
	301	+ 'asyncdownload' => 1,
	302	+ 'token' => $token,
	303	+ );
	304	+ if ( $ignoreWarnings ) {
	305	+ $params['ignorewarnings'] = 1;
	306	+ }
	307	+ if ( $leaveMessage ) {
	308	+ $params['leavemessage'] = 1;
	309	+ }
	310	+
	311	+ $data = $this->doApiRequest( $params );
	312	+ $this->assertEquals( $data[0]['upload']['result'], 'Queued' );
	313	+ $this->assertTrue( isset( $data[0]['upload']['statuskey'] ) );
	314	+ $statusKey = $data[0]['upload']['statuskey'];
	315	+
	316	+ $job = Job::pop();
	317	+ $this->assertEquals( 'UploadFromUrlJob', get_class( $job ) );
	318	+
	319	+ $status = $job->run();
	320	+ $this->assertTrue( $status );
	321	+
	322	+ $data = $this->doApiRequest( array(
	323	+ 'action' => 'upload',
	324	+ 'statuskey' => $statusKey,
	325	+ 'token' => $token,
	326	+ ) );
	327	+
	328	+ return $data;
	329	+ }
	330	+
207	331
208		~~- $this->assertTrue( $t->exists() );~~
209		-
210		~~- $this->deleteFile( 'UploadFromUrlTest.png' );~~
211		~~- }~~
212		-
213	332	/**
214	333	*
215	334	*/
216		~~- function deleteFile( $name ) {~~
	335	+ protected function deleteFile( $name ) {
217	336	$t = Title::newFromText( $name, NS_FILE );
218	337	$this->assertTrue($t->exists(), "File '$name' exists");
219	338
Index: trunk/phase3/includes/upload/UploadFromUrl.php
—	—	@@ -197,8 +197,10 @@
198	198	'userName' => $user->getName(),
199	199	'leaveMessage' => $this->mAsync == 'async-leavemessage',
200	200	'ignoreWarnings' => $this->mIgnoreWarnings,
	201	+ 'sessionId' => session_id(),
201	202	'sessionKey' => $sessionKey,
202	203	) );
	204	+ $job->initializeSessionData();
203	205	$job->insert();
204	206	return $sessionKey;
205	207	}
Index: trunk/phase3/includes/GlobalFunctions.php
—	—	@@ -2918,7 +2918,7 @@
2919	2919	/**
2920	2920	* Initialise php session
2921	2921	*/
2922		~~-function wfSetupSession() {~~
	2922	+function wfSetupSession( $sessionId = false ) {
2923	2923	global $wgSessionsInMemcached, $wgCookiePath, $wgCookieDomain,
2924	2924	$wgCookieSecure, $wgCookieHttpOnly, $wgSessionHandler;
2925	2925	if( $wgSessionsInMemcached ) {
—	—	@@ -2944,6 +2944,9 @@
2945	2945	session_set_cookie_params( 0, $wgCookiePath, $wgCookieDomain, $wgCookieSecure );
2946	2946	}
2947	2947	session_cache_limiter( 'private, must-revalidate' );
	2948	+ if ( $sessionId ) {
	2949	+ session_id( $sessionId );
	2950	+ }
2948	2951	wfSuppressWarnings();
2949	2952	session_start();
2950	2953	wfRestoreWarnings();
Index: trunk/phase3/includes/api/ApiUpload.php
—	—	@@ -55,7 +55,10 @@
56	56	$this->mParams['file'] = $request->getFileName( 'file' );
57	57
58	58	// Select an upload module
59		~~- $this->selectUploadModule();~~
	59	+ if ( !$this->selectUploadModule() ) {
	60	+ // This is not a true upload, but a status request or similar
	61	+ return;
	62	+ }
60	63	if ( !isset( $this->mUpload ) ) {
61	64	$this->dieUsage( 'No upload module set', 'nomodule' );
62	65	}
—	—	@@ -96,15 +99,39 @@
97	100	}
98	101
99	102	/**
100		~~- * Select an upload module and set it to mUpload. Dies on failure.~~
	103	+ * Select an upload module and set it to mUpload. Dies on failure. If the
	104	+ * request was a status request and not a true upload, returns false;
	105	+ * otherwise true
	106	+ *
	107	+ * @return bool
101	108	*/
102	109	protected function selectUploadModule() {
103	110	$request = $this->getMain()->getRequest();
104	111
105	112	// One and only one of the following parameters is needed
106	113	$this->requireOnlyOneParameter( $this->mParams,
107		~~- 'sessionkey', 'file', 'url' );~~
	114	+ 'sessionkey', 'file', 'url', 'statuskey' );
108	115
	116	+ if ( $this->mParams['statuskey'] ) {
	117	+ // Status request for an async upload
	118	+ $sessionData = UploadFromUrlJob::getSessionData( $this->mParams['statuskey'] );
	119	+ if ( !isset( $sessionData['result'] ) ) {
	120	+ $this->dieUsage();
	121	+ }
	122	+ if ( $sessionData['result'] == 'Warning' ) {
	123	+ $sessionData['warnings'] = $this->transformWarnings( $sessionData['warnings'] );
	124	+ $sessionData['sessionkey'] = $this->mParams['statuskey'];
	125	+ }
	126	+ $this->getResult()->addValue( null, $this->getModuleName(), $sessionData );
	127	+ return false;
	128	+
	129	+ }
	130	+
	131	+ // The following modules all require the filename parameter to be set
	132	+ if ( is_null( $this->mParams['filename'] ) ) {
	133	+ $this->dieUsageMsg( array( 'missingparam', 'filename' ) );
	134	+ }
	135	+
109	136	if ( $this->mParams['sessionkey'] ) {
110	137	// Upload stashed in a previous request
111	138	$sessionData = $request->getSessionData( UploadBase::getSessionKeyName() );
—	—	@@ -132,6 +159,11 @@
133	160
134	161	$async = false;
135	162	if ( $this->mParams['asyncdownload'] ) {
	163	+ if ( $this->mParams['leavemessage'] && !$this->mParams['ignorewarnings'] ) {
	164	+ $this->dieUsage( 'Using leavemessage without ignorewarnings is not supported',
	165	+ 'missing-ignorewarnings' );
	166	+ }
	167	+
136	168	if ( $this->mParams['leavemessage'] ) {
137	169	$async = 'async-leavemessage';
138	170	} else {
—	—	@@ -143,6 +175,8 @@
144	176	$this->mParams['url'], $async );
145	177
146	178	}
	179	+
	180	+ return true;
147	181	}
148	182
149	183	/**
—	—	@@ -225,25 +259,8 @@
226	260	if ( !$this->mParams['ignorewarnings'] ) {
227	261	$warnings = $this->mUpload->checkWarnings();
228	262	if ( $warnings ) {
229		~~- // Add indices~~
230		~~- $this->getResult()->setIndexedTagName( $warnings, 'warning' );~~
231		-
232		~~- if ( isset( $warnings['duplicate'] ) ) {~~
233		~~- $dupes = array();~~
234		~~- foreach ( $warnings['duplicate'] as $key => $dupe )~~
235		~~- $dupes[] = $dupe->getName();~~
236		~~- $this->getResult()->setIndexedTagName( $dupes, 'duplicate' );~~
237		~~- $warnings['duplicate'] = $dupes;~~
238		~~- }~~
239		-
240		~~- if ( isset( $warnings['exists'] ) ) {~~
241		~~- $warning = $warnings['exists'];~~
242		~~- unset( $warnings['exists'] );~~
243		~~- $warnings[$warning['warning']] = $warning['file']->getName();~~
244		~~- }~~
245		-
246	263	$result['result'] = 'Warning';
247		~~- $result['warnings'] = $warnings;~~
	264	+ $result['warnings'] = $this->transformWarnings( $warnings );
248	265
249	266	$sessionKey = $this->mUpload->stashSession();
250	267	if ( !$sessionKey ) {
—	—	@@ -257,7 +274,33 @@
258	275	}
259	276	return;
260	277	}
	278	+
	279	+ /**
	280	+ * Transforms a warnings array returned by mUpload->checkWarnings() into
	281	+ * something that can be directly used as API result
	282	+ */
	283	+ protected function transformWarnings( $warnings ) {
	284	+ // Add indices
	285	+ $this->getResult()->setIndexedTagName( $warnings, 'warning' );
261	286
	287	+ if ( isset( $warnings['duplicate'] ) ) {
	288	+ $dupes = array();
	289	+ foreach ( $warnings['duplicate'] as $key => $dupe ) {
	290	+ $dupes[] = $dupe->getName();
	291	+ }
	292	+ $this->getResult()->setIndexedTagName( $dupes, 'duplicate' );
	293	+ $warnings['duplicate'] = $dupes;
	294	+ }
	295	+
	296	+ if ( isset( $warnings['exists'] ) ) {
	297	+ $warning = $warnings['exists'];
	298	+ unset( $warnings['exists'] );
	299	+ $warnings[$warning['warning']] = $warning['file']->getName();
	300	+ }
	301	+
	302	+ return $warnings;
	303	+ }
	304	+
262	305	/**
263	306	* Perform the actual upload. Returns a suitable result array on success;
264	307	* dies on failure.
—	—	@@ -290,7 +333,7 @@
291	334	// requested so
292	335	return array(
293	336	'result' => 'Queued',
294		~~- 'sessionkey' => $error[0][1],~~
	337	+ 'statuskey' => $error[0][1],
295	338	);
296	339	} else {
297	340	$this->getResult()->setIndexedTagName( $error, 'error' );
—	—	@@ -320,7 +363,6 @@
321	364	$params = array(
322	365	'filename' => array(
323	366	ApiBase::PARAM_TYPE => 'string',
324		~~- ApiBase::PARAM_REQUIRED => true~~
325	367	),
326	368	'comment' => array(
327	369	ApiBase::PARAM_DFLT => ''
—	—	@@ -351,6 +393,7 @@
352	394	$params += array(
353	395	'asyncdownload' => false,
354	396	'leavemessage' => false,
	397	+ 'statuskey' => null,
355	398	);
356	399	}
357	400	return $params;
—	—	@@ -375,6 +418,7 @@
376	419	$params += array(
377	420	'asyncdownload' => 'Make fetching a URL asynchronous',
378	421	'leavemessage' => 'If asyncdownload is used, leave a message on the user talk page if finished',
	422	+ 'statuskey' => 'Fetch the upload status for this session key',
379	423	);
380	424	}
381	425
Index: trunk/phase3/includes/Wiki.php
—	—	@@ -379,6 +379,8 @@
380	380	$output->output();
381	381	// Do any deferred jobs
382	382	$this->doUpdates( $deferredUpdates );
	383	+ // Close the session so that jobs don't access the current session
	384	+ session_write_close();
383	385	$this->doJobs();
384	386	wfProfileOut( __METHOD__ );
385	387	}
Index: trunk/phase3/includes/job/UploadFromUrlJob.php
—	—	@@ -16,6 +16,8 @@
17	17	* @ingroup JobQueue
18	18	*/
19	19	class UploadFromUrlJob extends Job {
	20	+ const SESSION_KEYNAME = 'wsUploadFromUrlJobData';
	21	+
20	22	public $upload;
21	23	protected $user;
22	24
—	—	@@ -24,14 +26,6 @@
25	27	}
26	28
27	29	public function run() {
28		~~- # Until we find a way to store data in sessions, set leaveMessage to~~
29		~~- # true unconditionally~~
30		~~- $this->params['leaveMessage'] = true;~~
31		~~- # Similar for ignorewarnings. This is not really a good fallback, but~~
32		~~- # there is no easy way to get a wikitext formatted warning message to~~
33		~~- # show to the user~~
34		~~- $this->params['ignoreWarnings'] = true;~~
35		-
36	30	# Initialize this object and the upload object
37	31	$this->upload = new UploadFromUrl();
38	32	$this->upload->initialize(
—	—	@@ -60,6 +54,8 @@
61	55	if ( !$this->params['ignoreWarnings'] ) {
62	56	$warnings = $this->upload->checkWarnings();
63	57	if ( $warnings ) {
	58	+ wfSetupSession( $this->params['sessionId'] );
	59	+
64	60	if ( $this->params['leaveMessage'] ) {
65	61	$this->user->leaveUserMessage(
66	62	wfMsg( 'upload-warning-subj' ),
—	—	@@ -72,7 +68,10 @@
73	69	'warnings', $warnings );
74	70	}
75	71
76		~~- // FIXME: stash in session~~
	72	+ # Stash the upload in the session
	73	+ $this->upload->stashSession( $this->params['sessionKey'] );
	74	+ session_write_close();
	75	+
77	76	return true;
78	77	}
79	78	}
—	—	@@ -111,28 +110,44 @@
112	111	) );
113	112	}
114	113	} else {
	114	+ wfSetupSession( $this->params['sessionId'] );
115	115	if ( $status->isOk() ) {
116	116	$this->storeResultInSession( 'Success',
117		~~- 'filename', $this->getLocalFile()->getName() );~~
	117	+ 'filename', $this->upload->getLocalFile()->getName() );
118	118	} else {
119	119	$this->storeResultInSession( 'Failure',
120	120	'errors', $status->getErrorsArray() );
121	121	}
122		-
	122	+ session_write_close();
123	123	}
124	124	}
125	125
126	126	/**
127		~~- * Store a result in the session data~~
128		~~- * THIS IS BROKEN. $_SESSION does not exist when using runJobs.php~~
	127	+ * Store a result in the session data. Note that the caller is responsible
	128	+ * for appropriate session_start and session_write_close calls.
129	129	*
130	130	* @param $result String: the result (Success\|Warning\|Failure)
131	131	* @param $dataKey String: the key of the extra data
132	132	* @param $dataValue Mixed: the extra data itself
133	133	*/
134	134	protected function storeResultInSession( $result, $dataKey, $dataValue ) {
135		~~- $session &= $_SESSION[UploadBase::getSessionKeyname()][$this->params['sessionKey']];~~
	135	+ $session =& self::getSessionData( $this->params['sessionKey'] );
136	136	$session['result'] = $result;
137	137	$session[$dataKey] = $dataValue;
138	138	}
	139	+
	140	+ /**
	141	+ * Initialize the session data. Sets the intial result to queued.
	142	+ */
	143	+ public function initializeSessionData() {
	144	+ $session =& self::getSessionData( $this->params['sessionKey'] );
	145	+ $$session['result'] = 'Queued';
	146	+ }
	147	+
	148	+ public static function &getSessionData( $key ) {
	149	+ if ( !isset( $_SESSION[self::SESSION_KEYNAME][$key] ) ) {
	150	+ $_SESSION[self::SESSION_KEYNAME][$key] = array();
	151	+ }
	152	+ return $_SESSION[self::SESSION_KEYNAME][$key];
	153	+ }
139	154	}
Index: trunk/phase3/RELEASE-NOTES
—	—	@@ -380,6 +380,8 @@
381	381	missing
382	382	* (bug 24724) list=allusers is out by 1 (shows total users - 1)
383	383	* (bug 24166) API error when using rvprop=tags
	384	+* Introduced "asynchronous download" mode for upload-by-url. Requires
	385	+ $wgAllowAsyncCopyUploads to be true.
384	386
385	387	=== Languages updated in 1.17 ===
386	388

Follow-up revisions

Revision	Commit summary	Author	Date
r72578	Followup r72475: assert that a job has been popped to avoid fatals	btongminh	11:25, 8 September 2010
r72710	Follow-up r72475: Set $wgAllowAsyncCopyUploads in the test	btongminh	11:17, 10 September 2010
r75588	Add feature to block common (weak) passwords....	platonides	22:26, 27 October 2010
r75589	Do NOT use hardcoded passwords! Not even if the user agreed to run destructiv...	platonides	22:27, 27 October 2010
r87235	Remove the session_write_close() from r72475 since it apparently causes bug 2...	tstarling	04:21, 2 May 2011
r88904	(bug 27891) Follow-up r72475: destroy the LBFactory singleton before doing an...	btongminh	17:11, 26 May 2011

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r70137	Made asynchronous upload by URL working, partly. Hid it behind $wgAllowAsyncC...	btongminh	13:53, 29 July 2010

Comments

#Comment by 😂 (talk | contribs) 15:42, 7 September 2010

PHP Fatal error:  Call to a member function run() on a non-object in /opt/local/apache2/htdocs/phase3/maintenance/tests/UploadFromUrlTest.php on line 247
PHP Stack trace:
PHP   1. {main}() /opt/local/apache2/htdocs/phase3/maintenance/tests/phpunit:0
PHP   2. PHPUnit_TextUI_Command->run() /opt/local/apache2/htdocs/phase3/maintenance/tests/phpunit:53
PHP   3. PHPUnit_TextUI_TestRunner->doRun() /opt/local/PEAR/PHPUnit/TextUI/Command.php:213
PHP   4. PHPUnit_Framework_TestSuite->run() /opt/local/PEAR/PHPUnit/TextUI/TestRunner.php:349
PHP   5. PHPUnit_Framework_TestSuite->run() /opt/local/PEAR/PHPUnit/Framework/TestSuite.php:674
PHP   6. PHPUnit_Framework_TestSuite->runTest() /opt/local/PEAR/PHPUnit/Framework/TestSuite.php:731
PHP   7. PHPUnit_Framework_TestCase->run() /opt/local/PEAR/PHPUnit/Framework/TestSuite.php:756
PHP   8. PHPUnit_Framework_TestResult->run() /opt/local/PEAR/PHPUnit/Framework/TestCase.php:652
PHP   9. PHPUnit_Framework_TestCase->runBare() /opt/local/PEAR/PHPUnit/Framework/TestResult.php:686
PHP  10. PHPUnit_Framework_TestCase->runTest() /opt/local/PEAR/PHPUnit/Framework/TestCase.php:705
PHP  11. ReflectionMethod->invokeArgs() /opt/local/PEAR/PHPUnit/Framework/TestCase.php:822
PHP  12. UploadFromUrlTest->testLeaveMessage() /opt/local/apache2/htdocs/phase3/maintenance/tests/UploadFromUrlTest.php:0

#Comment by 😂 (talk | contribs) 18:07, 7 September 2010

Also ending up with some new rows in recentchanges.

#Comment by Bryan (talk | contribs) 11:30, 8 September 2010

I added an assertion in r72578. This should fix the fatal, but will probably cause the test to fail for you. (It doesn't for me) Could you check?

#Comment by 😂 (talk | contribs) 15:10, 8 September 2010

Fatal is gone, but I'm getting some exceptions now. Full output from $ phpunit --configuration suite.xml --filter "UploadFromUrl*" at http://dpaste.org/AwFp/

#Comment by 😂 (talk | contribs) 15:11, 8 September 2010

Code Review url parsing is retarded. Put a / on the end of the URL

#Comment by Bryan (talk | contribs) 11:18, 10 September 2010

Fixed in r72578.

#Comment by Bryan (talk | contribs) 11:18, 10 September 2010

Actually r72710.

#Comment by 😂 (talk | contribs) 11:31, 10 September 2010

Confirmed that works now. Thanks :)

#Comment by NeilK (talk | contribs) 21:20, 24 September 2010

If I don't have the global $wgAllowAsyncCopyUploads set to true, this error message can occur:

  Notice: Undefined index: statuskey in /Users/neilk/Sites/wiki/includes/api/ApiUpload.php on line 191

I'm not sure about this idea of adding parameters conditionally... one of the main assumptions throughout the API code is that all potential parameters are initialized, even if it's just to null. This makes coding a lot simpler.

I quickly grepped through all the api/Api*.php modules and I don't see any other conditionally added parameters.[1]

I'm in the same boat as you with UploadWizard, I have to add parameters too, but I was thinking of doing so via a hook from an extension (but the parameters would then always be present and defined even if the extension was not turned on).

[1] neilk@ivy:~/Sites/trunk/includes/api$ perl -wlne 'print if (/^(\s*).*function\s+getAllowedParams/ .. /$1}/)' *.php | less

#Comment by NeilK (talk | contribs) 21:27, 24 September 2010

oh, this was actually added in r70137. Well, anyway same comment.

#Comment by Tim Starling (talk | contribs) 04:13, 2 May 2011

Moving the session_write_close() up completely broke ChronologyProtector and so caused bug 27891.

#Comment by Bryan (talk | contribs) 17:12, 26 May 2011

Fixed in r88904.

Status & tagging log

17:46, 27 June 2011 Reedy (talk | contribs) changed the status of r72475 [removed: new added: resolved]
17:12, 26 May 2011 Bryan (talk | contribs) changed the status of r72475 [removed: fixme added: new]
04:13, 2 May 2011 Tim Starling (talk | contribs) changed the status of r72475 [removed: ok added: fixme]
18:56, 13 January 2011 MarkAHershberger (talk | contribs) changed the tags for r72475 [removed: hexmode]
18:56, 13 January 2011 MarkAHershberger (talk | contribs) changed the status of r72475 [removed: new added: ok]
01:03, 13 January 2011 MarkAHershberger (talk | contribs) changed the tags for r72475 [added: hexmode]
11:18, 10 September 2010 Bryan (talk | contribs) changed the status of r72475 [removed: fixme added: new]
15:42, 7 September 2010 😂 (talk | contribs) changed the status of r72475 [removed: new added: fixme]