r70461 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r70460‎ \| r70461 \| r70462 >
Date:	14:15, 4 August 2010
Author:	soxred93
Status:	ok (Comments)
Tags:
Comment:	Followup to r70460: Committed wrong version of ApiBase.php, convert all core API modules to PARAM_REQUIRED syntax
Modified paths:	/trunk/phase3/includes/api/ApiBase.php (modified) (history) /trunk/phase3/includes/api/ApiBlock.php (modified) (history) /trunk/phase3/includes/api/ApiEditPage.php (modified) (history) /trunk/phase3/includes/api/ApiEmailUser.php (modified) (history) /trunk/phase3/includes/api/ApiMove.php (modified) (history) /trunk/phase3/includes/api/ApiPatrol.php (modified) (history) /trunk/phase3/includes/api/ApiProtect.php (modified) (history) /trunk/phase3/includes/api/ApiPurge.php (modified) (history) /trunk/phase3/includes/api/ApiQueryBacklinks.php (modified) (history) /trunk/phase3/includes/api/ApiRollback.php (modified) (history) /trunk/phase3/includes/api/ApiUndelete.php (modified) (history) /trunk/phase3/includes/api/ApiUpload.php (modified) (history) /trunk/phase3/includes/api/ApiUserrights.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiMove.php
—	—	@@ -45,9 +45,6 @@
46	46	}
47	47
48	48	$this->requireOnlyOneParameter( $params, 'from', 'fromid' );
49		~~- if ( !isset( $params['to'] ) ) {~~
50		~~- $this->dieUsageMsg( array( 'missingparam', 'to' ) );~~
51		~~- }~~
52	49
53	50	if ( isset( $params['from'] ) ) {
54	51	$fromTitle = Title::newFromText( $params['from'] );
—	—	@@ -172,7 +169,10 @@
173	170	'fromid' => array(
174	171	ApiBase::PARAM_TYPE => 'integer'
175	172	),
176		~~- 'to' => null,~~
	173	+ 'to' => array(
	174	+ ApiBase::PARAM_TYPE => 'string',
	175	+ ApiBase::PARAM_REQUIRED => 1
	176	+ ),
177	177	'token' => null,
178	178	'reason' => null,
179	179	'movetalk' => false,
Index: trunk/phase3/includes/api/ApiProtect.php
—	—	@@ -41,12 +41,6 @@
42	42	$params = $this->extractRequestParams();
43	43
44	44	$titleObj = null;
45		~~- if ( !isset( $params['title'] ) ) {~~
46		~~- $this->dieUsageMsg( array( 'missingparam', 'title' ) );~~
47		~~- }~~
48		~~- if ( empty( $params['protections'] ) ) {~~
49		~~- $this->dieUsageMsg( array( 'missingparam', 'protections' ) );~~
50		~~- }~~
51	45
52	46	$titleObj = Title::newFromText( $params['title'] );
53	47	if ( !$titleObj ) {
—	—	@@ -149,10 +143,14 @@
150	144
151	145	public function getAllowedParams() {
152	146	return array(
153		~~- 'title' => null,~~
	147	+ 'title' => array(
	148	+ ApiBase::PARAM_TYPE => 'string',
	149	+ ApiBase::PARAM_REQUIRED => 1
	150	+ ),
154	151	'token' => null,
155	152	'protections' => array(
156		~~- ApiBase::PARAM_ISMULTI => true~~
	153	+ ApiBase::PARAM_ISMULTI => true,
	154	+ ApiBase::PARAM_REQUIRED => 1,
157	155	),
158	156	'expiry' => array(
159	157	ApiBase::PARAM_ISMULTI => true,
Index: trunk/phase3/includes/api/ApiPurge.php
—	—	@@ -46,9 +46,6 @@
47	47	if ( !$wgUser->isAllowed( 'purge' ) ) {
48	48	$this->dieUsageMsg( array( 'cantpurge' ) );
49	49	}
50		~~- if ( !isset( $params['titles'] ) ) {~~
51		~~- $this->dieUsageMsg( array( 'missingparam', 'titles' ) );~~
52		~~- }~~
53	50	$result = array();
54	51	foreach ( $params['titles'] as $t ) {
55	52	$r = array();
—	—	@@ -86,7 +83,8 @@
87	84	public function getAllowedParams() {
88	85	return array(
89	86	'titles' => array(
90		~~- ApiBase::PARAM_ISMULTI => true~~
	87	+ ApiBase::PARAM_ISMULTI => true,
	88	+ ApiBase::PARAM_REQUIRED => 1
91	89	)
92	90	);
93	91	}
Index: trunk/phase3/includes/api/ApiQueryBacklinks.php
—	—	@@ -345,8 +345,6 @@
346	346	} else {
347	347	$this->rootTitle = $title;
348	348	}
349		~~- } else {~~
350		~~- $this->dieUsageMsg( array( 'missingparam', 'title' ) );~~
351	349	}
352	350	}
353	351
—	—	@@ -404,7 +402,10 @@
405	403
406	404	public function getAllowedParams() {
407	405	$retval = array(
408		~~- 'title' => null,~~
	406	+ 'title' => array(
	407	+ ApiBase::PARAM_TYPE => 'string',
	408	+ ApiBase::PARAM_REQUIRED => 1
	409	+ ),
409	410	'continue' => null,
410	411	'namespace' => array(
411	412	ApiBase::PARAM_ISMULTI => true,
Index: trunk/phase3/includes/api/ApiRollback.php
—	—	@@ -77,8 +77,14 @@
78	78
79	79	public function getAllowedParams() {
80	80	return array(
81		~~- 'title' => null,~~
82		~~- 'user' => null,~~
	81	+ 'title' => array(
	82	+ ApiBase::PARAM_TYPE => 'string',
	83	+ ApiBase::PARAM_REQUIRED => 1
	84	+ ),
	85	+ 'user' => array(
	86	+ ApiBase::PARAM_TYPE => 'string',
	87	+ ApiBase::PARAM_REQUIRED => 1
	88	+ ),
83	89	'token' => null,
84	90	'summary' => null,
85	91	'markbot' => false,
—	—	@@ -133,10 +139,6 @@
134	140
135	141	$params = $this->extractRequestParams();
136	142
137		~~- if ( !isset( $params['user'] ) ) {~~
138		~~- $this->dieUsageMsg( array( 'missingparam', 'user' ) );~~
139		~~- }~~
140		-
141	143	// We need to be able to revert IPs, but getCanonicalName rejects them
142	144	$this->mUser = User::isIP( $params['user'] )
143	145	? $params['user']
—	—	@@ -154,9 +156,6 @@
155	157	}
156	158
157	159	$params = $this->extractRequestParams();
158		~~- if ( !isset( $params['title'] ) ) {~~
159		~~- $this->dieUsageMsg( array( 'missingparam', 'title' ) );~~
160		~~- }~~
161	160
162	161	$this->mTitleObj = Title::newFromText( $params['title'] );
163	162
Index: trunk/phase3/includes/api/ApiUserrights.php
—	—	@@ -61,9 +61,6 @@
62	62	}
63	63
64	64	$params = $this->extractRequestParams();
65		~~- if ( is_null( $params['user'] ) ) {~~
66		~~- $this->dieUsageMsg( array( 'missingparam', 'user' ) );~~
67		~~- }~~
68	65
69	66	$form = new UserrightsPage;
70	67	$status = $form->fetchUser( $params['user'] );
—	—	@@ -88,7 +85,10 @@
89	86
90	87	public function getAllowedParams() {
91	88	return array (
92		~~- 'user' => null,~~
	89	+ 'user' => array(
	90	+ ApiBase::PARAM_TYPE => 'string',
	91	+ ApiBase::PARAM_REQUIRED => 1
	92	+ ),
93	93	'add' => array(
94	94	ApiBase::PARAM_TYPE => User::getAllGroups(),
95	95	ApiBase::PARAM_ISMULTI => true
Index: trunk/phase3/includes/api/ApiEditPage.php
—	—	@@ -45,10 +45,6 @@
46	46	global $wgUser;
47	47	$params = $this->extractRequestParams();
48	48
49		~~- if ( is_null( $params['title'] ) ) {~~
50		~~- $this->dieUsageMsg( array( 'missingparam', 'title' ) );~~
51		~~- }~~
52		-
53	49	if ( is_null( $params['text'] ) && is_null( $params['appendtext'] ) &&
54	50	is_null( $params['prependtext'] ) &&
55	51	$params['undo'] == 0 )
—	—	@@ -388,7 +384,10 @@
389	385
390	386	protected function getAllowedParams() {
391	387	return array(
392		~~- 'title' => null,~~
	388	+ 'title' => array(
	389	+ ApiBase::PARAM_TYPE => 'string',
	390	+ ApiBase::PARAM_REQUIRED => 1
	391	+ ),
393	392	'section' => null,
394	393	'text' => null,
395	394	'token' => null,
Index: trunk/phase3/includes/api/ApiEmailUser.php
—	—	@@ -41,13 +41,6 @@
42	42	global $wgUser;
43	43
44	44	$params = $this->extractRequestParams();
45		~~- // Check required parameters~~
46		~~- if ( !isset( $params['target'] ) ) {~~
47		~~- $this->dieUsageMsg( array( 'missingparam', 'target' ) );~~
48		~~- }~~
49		~~- if ( !isset( $params['text'] ) ) {~~
50		~~- $this->dieUsageMsg( array( 'missingparam', 'text' ) );~~
51		~~- }~~
52	45
53	46	// Validate target
54	47	$targetUser = SpecialEmailUser::getTarget( $params['target'] );
—	—	@@ -90,9 +83,15 @@
91	84
92	85	public function getAllowedParams() {
93	86	return array(
94		~~- 'target' => null,~~
	87	+ 'target' => array(
	88	+ ApiBase::PARAM_TYPE => 'string',
	89	+ ApiBase::PARAM_REQUIRED => 1
	90	+ ),
95	91	'subject' => null,
96		~~- 'text' => null,~~
	92	+ 'text' => array(
	93	+ ApiBase::PARAM_TYPE => 'string',
	94	+ ApiBase::PARAM_REQUIRED => 1
	95	+ ),
97	96	'token' => null,
98	97	'ccme' => false,
99	98	);
Index: trunk/phase3/includes/api/ApiBlock.php
—	—	@@ -58,9 +58,6 @@
59	59	return;
60	60	}
61	61
62		~~- if ( is_null( $params['user'] ) ) {~~
63		~~- $this->dieUsageMsg( array( 'missingparam', 'user' ) );~~
64		~~- }~~
65	62	if ( !$wgUser->isAllowed( 'block' ) ) {
66	63	$this->dieUsageMsg( array( 'cantblock' ) );
67	64	}
—	—	@@ -135,7 +132,10 @@
136	133
137	134	public function getAllowedParams() {
138	135	return array(
139		~~- 'user' => null,~~
	136	+ 'user' => array(
	137	+ ApiBase::PARAM_TYPE => 'string',
	138	+ ApiBase::PARAM_REQUIRED => 1
	139	+ ),
140	140	'token' => null,
141	141	'gettoken' => false,
142	142	'expiry' => 'never',
Index: trunk/phase3/includes/api/ApiPatrol.php
—	—	@@ -43,10 +43,6 @@
44	44	public function execute() {
45	45	$params = $this->extractRequestParams();
46	46
47		~~- if ( !isset( $params['rcid'] ) ) {~~
48		~~- $this->dieUsageMsg( array( 'missingparam', 'rcid' ) );~~
49		~~- }~~
50		-
51	47	$rc = RecentChange::newFromID( $params['rcid'] );
52	48	if ( !$rc instanceof RecentChange ) {
53	49	$this->dieUsageMsg( array( 'nosuchrcid', $params['rcid'] ) );
—	—	@@ -70,7 +66,8 @@
71	67	return array(
72	68	'token' => null,
73	69	'rcid' => array(
74		~~- ApiBase::PARAM_TYPE => 'integer'~~
	70	+ ApiBase::PARAM_TYPE => 'integer',
	71	+ ApiBase::PARAM_REQUIRED => 1
75	72	),
76	73	);
77	74	}
Index: trunk/phase3/includes/api/ApiUndelete.php
—	—	@@ -41,9 +41,6 @@
42	42	$params = $this->extractRequestParams();
43	43
44	44	$titleObj = null;
45		~~- if ( !isset( $params['title'] ) ) {~~
46		~~- $this->dieUsageMsg( array( 'missingparam', 'title' ) );~~
47		~~- }~~
48	45
49	46	if ( !$wgUser->isAllowed( 'undelete' ) ) {
50	47	$this->dieUsageMsg( array( 'permdenied-undelete' ) );
—	—	@@ -101,7 +98,10 @@
102	99
103	100	public function getAllowedParams() {
104	101	return array(
105		~~- 'title' => null,~~
	102	+ 'title' => array(
	103	+ ApiBase::PARAM_TYPE => 'string',
	104	+ ApiBase::PARAM_REQUIRED => 1
	105	+ ),
106	106	'token' => null,
107	107	'reason' => '',
108	108	'timestamps' => array(
Index: trunk/phase3/includes/api/ApiBase.php
—	—	@@ -503,7 +503,7 @@
504	504
505	505	$allparams = $this->getAllowedParams();
506	506	foreach( $this->mParamCache[$parseLimit] as $param => $val ) {
507		~~- if( !isset( $allparams[$param][ApiBase::PARAM_REQUIRED] ) ) {~~
	507	+ if( isset( $allparams[$param][ApiBase::PARAM_REQUIRED] ) && !isset( $val ) ) {
508	508	$this->dieUsageMsg( array( 'missingparam', $param ) );
509	509	}
510	510	}
Index: trunk/phase3/includes/api/ApiUpload.php
—	—	@@ -101,10 +101,6 @@
102	102	// One and only one of the following parameters is needed
103	103	$this->requireOnlyOneParameter( $this->mParams,
104	104	'sessionkey', 'file', 'url' );
105		~~- // And this one is needed~~
106		~~- if ( !isset( $this->mParams['filename'] ) ) {~~
107		~~- $this->dieUsageMsg( array( 'missingparam', 'filename' ) );~~
108		~~- }~~
109	105
110	106	if ( $this->mParams['sessionkey'] ) {
111	107	// Upload stashed in a previous request
—	—	@@ -318,7 +314,10 @@
319	315
320	316	public function getAllowedParams() {
321	317	$params = array(
322		~~- 'filename' => null,~~
	318	+ 'filename' => array(
	319	+ ApiBase::PARAM_TYPE => 'string',
	320	+ ApiBase::PARAM_REQUIRED => 1
	321	+ ),
323	322	'comment' => array(
324	323	ApiBase::PARAM_DFLT => ''
325	324	),

Follow-up revisions

Revision	Commit summary	Author	Date
r70462	Followup to r70460 and r70461: Use true instead of 1	soxred93	14:29, 4 August 2010
r70474	Followup r70461 if PARAM_REQUIRED is set, use for missing param in getPossibl...	reedy	19:20, 4 August 2010
r70476	Followup r70474 and r70461, drop missingparam from getPossibleErrors	reedy	19:32, 4 August 2010
r70477	Followup r70460/r70461...	reedy	20:27, 4 August 2010
r70478	Make search enforced by API in search by setting PARAM_REQUIRED...	reedy	20:47, 4 August 2010
r70479	Further followup to r70460/r70461 and r70477...	reedy	21:19, 4 August 2010
r73469	1.16wmf4: Merging PARAM_REQUIRED support from trunk for upcoming ArticleAsses...	catrope	18:53, 21 September 2010

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r70460	* PARAM_REQUIRED parameter flag added. If this flag is set, and the end user ...	soxred93	13:35, 4 August 2010

Comments

#Comment by Reedy (talk | contribs) 20:25, 4 August 2010

You've actually unknowingly fixed a bug here.

Title should've been a required paramter in Backlinks via any method, but it wasn't

Status & tagging log

21:28, 4 August 2010 Reedy (talk | contribs) changed the status of r70461 [removed: new added: ok]