r64197 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r64196‎ \| r64197 \| r64198 >
Date:	22:15, 25 March 2010
Author:	reedy
Status:	resolved (Comments)
Tags:
Comment:	Fix bug 22944 in a much better fashion (using watchlist parameter) Deprecate old watch/unwatch parameters Move generic watchlist stuff to ApiBase/getWatchlistValue (maybe needs renaming better?) Tweak some braces in ApiEditPage
Modified paths:	/trunk/phase3/RELEASE-NOTES (modified) (history) /trunk/phase3/includes/api/ApiBase.php (modified) (history) /trunk/phase3/includes/api/ApiDelete.php (modified) (history) /trunk/phase3/includes/api/ApiEditPage.php (modified) (history) /trunk/phase3/includes/api/ApiMove.php (modified) (history) /trunk/phase3/includes/api/ApiProtect.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)

Diff [purge]

Index: trunk/phase3/includes/api/ApiMove.php
—	—	@@ -121,12 +121,15 @@
122	122	$this->getResult()->setIndexedTagName( $r['subpages-talk'], 'subpage' );
123	123	}
124	124	}
	125	+
	126	+ // Watch pages
	127	+ $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj ) \|\| $wgUser->getOption( 'watchmoves' );
125	128
126		~~- // Watch pages~~
127		~~- if ( $params['watch'] \|\| $wgUser->getOption( 'watchmoves' ) ) {~~
	129	+ // Deprecated parameters
	130	+ if ( $params['watch'] \|\| $watch ) {
128	131	$wgUser->addWatch( $fromTitle );
129	132	$wgUser->addWatch( $toTitle );
130		~~- } elseif ( $params['unwatch'] ) {~~
	133	+ } elseif ( $params['unwatch'] \|\| !$watch ) {
131	134	$wgUser->removeWatch( $fromTitle );
132	135	$wgUser->removeWatch( $toTitle );
133	136	}
—	—	@@ -175,8 +178,23 @@
176	179	'movetalk' => false,
177	180	'movesubpages' => false,
178	181	'noredirect' => false,
179		~~- 'watch' => false,~~
180		~~- 'unwatch' => false,~~
	182	+ 'watch' => array(
	183	+ ApiBase::PARAM_DFLT => false,
	184	+ ApiBase::PARAM_DEPRECATED => true,
	185	+ ),
	186	+ 'unwatch' => array(
	187	+ ApiBase::PARAM_DFLT => false,
	188	+ ApiBase::PARAM_DEPRECATED => true,
	189	+ ),
	190	+ 'watchlist' => array(
	191	+ ApiBase::PARAM_DFLT => 'preferences',
	192	+ ApiBase::PARAM_TYPE => array(
	193	+ 'watch',
	194	+ 'unwatch',
	195	+ 'preferences',
	196	+ 'nochange'
	197	+ ),
	198	+ ),
181	199	'ignorewarnings' => false
182	200	);
183	201	}
—	—	@@ -193,6 +211,7 @@
194	212	'noredirect' => 'Don\'t create a redirect',
195	213	'watch' => 'Add the page and the redirect to your watchlist',
196	214	'unwatch' => 'Remove the page and the redirect from your watchlist',
	215	+ 'watchlist' => 'Unconditionally add or remove the page from your watchlist, use preferences or do not change watch',
197	216	'ignorewarnings' => 'Ignore any warnings'
198	217	);
199	218	}
Index: trunk/phase3/includes/api/ApiProtect.php
—	—	@@ -113,9 +113,15 @@
114	114
115	115	$cascade = $params['cascade'];
116	116	$articleObj = new Article( $titleObj );
117		~~- if ( $params['watch'] ) {~~
	117	+
	118	+ $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj );
	119	+
	120	+ if ( $params['watch'] \|\| $watch ) {
118	121	$articleObj->doWatch();
	122	+ } else if ( !$watch ) {
	123	+ $articleObj->doUnwatch();
119	124	}
	125	+
120	126	if ( $titleObj->exists() ) {
121	127	$ok = $articleObj->updateRestrictions( $protections, $params['reason'], $cascade, $expiryarray );
122	128	} else {
—	—	@@ -160,7 +166,19 @@
161	167	),
162	168	'reason' => '',
163	169	'cascade' => false,
164		~~- 'watch' => false,~~
	170	+ 'watch' => array(
	171	+ ApiBase::PARAM_DFLT => false,
	172	+ ApiBase::PARAM_DEPRECATED => true,
	173	+ ),
	174	+ 'watchlist' => array(
	175	+ ApiBase::PARAM_DFLT => 'preferences',
	176	+ ApiBase::PARAM_TYPE => array(
	177	+ 'watch',
	178	+ 'unwatch',
	179	+ 'preferences',
	180	+ 'nochange'
	181	+ ),
	182	+ ),
165	183	);
166	184	}
167	185
—	—	@@ -175,6 +193,7 @@
176	194	'cascade' => array( 'Enable cascading protection (i.e. protect pages included in this page)',
177	195	'Ignored if not all protection levels are \'sysop\' or \'protect\'' ),
178	196	'watch' => 'If set, add the page being (un)protected to your watchlist',
	197	+ 'watchlist' => 'Unconditionally add or remove the page from your watchlist, use preferences or do not change watch',
179	198	);
180	199	}
181	200
Index: trunk/phase3/includes/api/ApiRollback.php
—	—	@@ -72,6 +72,14 @@
73	73	// We don't care about multiple errors, just report one of them
74	74	$this->dieUsageMsg( reset( $retval ) );
75	75	}
	76	+
	77	+ $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj );
	78	+
	79	+ if ( $watch ) {
	80	+ $articleObj->doWatch();
	81	+ } else if ( !$watch ) {
	82	+ $articleObj->doUnwatch();
	83	+ }
76	84
77	85	$info = array(
78	86	'title' => $titleObj->getPrefixedText(),
—	—	@@ -99,7 +107,16 @@
100	108	'user' => null,
101	109	'token' => null,
102	110	'summary' => null,
103		~~- 'markbot' => false~~
	111	+ 'markbot' => false,
	112	+ 'watchlist' => array(
	113	+ ApiBase::PARAM_DFLT => 'preferences',
	114	+ ApiBase::PARAM_TYPE => array(
	115	+ 'watch',
	116	+ 'unwatch',
	117	+ 'preferences',
	118	+ 'nochange'
	119	+ ),
	120	+ ),
104	121	);
105	122	}
106	123
—	—	@@ -109,7 +126,8 @@
110	127	'user' => 'Name of the user whose edits are to be rolled back. If set incorrectly, you\'ll get a badtoken error.',
111	128	'token' => 'A rollback token previously retrieved through prop=revisions',
112	129	'summary' => 'Custom edit summary. If not set, default summary will be used.',
113		~~- 'markbot' => 'Mark the reverted edits and the revert as bot edits'~~
	130	+ 'markbot' => 'Mark the reverted edits and the revert as bot edits',
	131	+ 'watchlist' => 'Unconditionally add or remove the page from your watchlist, use preferences or do not change watch',
114	132	);
115	133	}
116	134
Index: trunk/phase3/includes/api/ApiDelete.php
—	—	@@ -81,10 +81,13 @@
82	82	if ( count( $retval ) ) {
83	83	$this->dieUsageMsg( reset( $retval ) ); // We don't care about multiple errors, just report one of them
84	84	}
	85	+
	86	+ $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj ) \|\| $wgUser->getOption( 'watchdeletion' );
85	87
86		~~- if ( $params['watch'] \|\| $wgUser->getOption( 'watchdeletion' ) ) {~~
	88	+ // Deprecated parameters
	89	+ if ( $params['watch'] \|\| $watch ) {
87	90	$articleObj->doWatch();
88		~~- } elseif ( $params['unwatch'] ) {~~
	91	+ } elseif ( $params['unwatch'] \|\| !$watch ) {
89	92	$articleObj->doUnwatch();
90	93	}
91	94	}
—	—	@@ -197,7 +200,19 @@
198	201	),
199	202	'token' => null,
200	203	'reason' => null,
201		~~- 'watch' => false,~~
	204	+ 'watch' => array(
	205	+ ApiBase::PARAM_DFLT => false,
	206	+ ApiBase::PARAM_DEPRECATED => true,
	207	+ ),
	208	+ 'watchlist' => array(
	209	+ ApiBase::PARAM_DFLT => 'preferences',
	210	+ ApiBase::PARAM_TYPE => array(
	211	+ 'watch',
	212	+ 'unwatch',
	213	+ 'preferences',
	214	+ 'nochange'
	215	+ ),
	216	+ ),
202	217	'unwatch' => false,
203	218	'oldimage' => null
204	219	);
—	—	@@ -210,6 +225,7 @@
211	226	'token' => 'A delete token previously retrieved through prop=info',
212	227	'reason' => 'Reason for the deletion. If not set, an automatically generated reason will be used.',
213	228	'watch' => 'Add the page to your watchlist',
	229	+ 'watchlist' => 'Unconditionally add or remove the page from your watchlist, use preferences or do not change watch',
214	230	'unwatch' => 'Remove the page from your watchlist',
215	231	'oldimage' => 'The name of the old image to delete as provided by iiprop=archivename'
216	232	);
Index: trunk/phase3/includes/api/ApiEditPage.php
—	—	@@ -177,15 +177,13 @@
178	178	$reqArr['wpEdittime'] = $articleObj->getTimestamp();
179	179	}
180	180
181		~~- if ( !is_null( $params['starttimestamp'] ) && $params['starttimestamp'] != '' )~~
182		~~- {~~
	181	+ if ( !is_null( $params['starttimestamp'] ) && $params['starttimestamp'] != '' ) {
183	182	$reqArr['wpStarttime'] = wfTimestamp( TS_MW, $params['starttimestamp'] );
184	183	} else {
185	184	$reqArr['wpStarttime'] = $reqArr['wpEdittime']; // Fake wpStartime
186	185	}
187	186
188		~~- if ( $params['minor'] \|\| ( !$params['notminor'] && $wgUser->getOption( 'minordefault' ) ) )~~
189		~~- {~~
	187	+ if ( $params['minor'] \|\| ( !$params['notminor'] && $wgUser->getOption( 'minordefault' ) ) ) {
190	188	$reqArr['wpMinoredit'] = '';
191	189	}
192	190
—	—	@@ -204,25 +202,8 @@
205	203	$reqArr['wpSection'] = '';
206	204	}
207	205
208		~~- // Handle watchlist settings~~
209		~~- switch ( $params['watchlist'] ) {~~
210		~~- case 'watch':~~
211		~~- $watch = true;~~
212		~~- break;~~
213		~~- case 'unwatch':~~
214		~~- $watch = false;~~
215		~~- break;~~
216		~~- case 'preferences':~~
217		~~- if ( $titleObj->exists() ) {~~
218		~~- $watch = $wgUser->getOption( 'watchdefault' ) \|\| $titleObj->userIsWatching();~~
219		~~- } else {~~
220		~~- $watch = $wgUser->getOption( 'watchcreations' );~~
221		~~- }~~
222		~~- break;~~
223		~~- case 'nochange':~~
224		~~- default:~~
225		~~- $watch = $titleObj->userIsWatching();~~
226		~~- }~~
	206	+ $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj ) \|\| $wgUser->getOption( 'watchcreations' );
	207	+
227	208	// Deprecated parameters
228	209	if ( $params['watch'] ) {
229	210	$watch = true;
Index: trunk/phase3/includes/api/ApiUndelete.php
—	—	@@ -81,6 +81,14 @@
82	82	wfRunHooks( 'FileUndeleteComplete',
83	83	array( $titleObj, array(), $wgUser, $params['reason'] ) );
84	84	}
	85	+
	86	+ $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj );
	87	+
	88	+ if ( $params['watch'] \|\| $watch ) {
	89	+ $wgUser->addWatch( $titleObj );
	90	+ } else if ( !$watch ) {
	91	+ $wgUser->removeWatch( $titleObj );
	92	+ }
85	93
86	94	$info['title'] = $titleObj->getPrefixedText();
87	95	$info['revisions'] = intval( $retval[0] );
—	—	@@ -104,7 +112,16 @@
105	113	'reason' => '',
106	114	'timestamps' => array(
107	115	ApiBase::PARAM_ISMULTI => true
108		~~- )~~
	116	+ ),
	117	+ 'watchlist' => array(
	118	+ ApiBase::PARAM_DFLT => 'preferences',
	119	+ ApiBase::PARAM_TYPE => array(
	120	+ 'watch',
	121	+ 'unwatch',
	122	+ 'preferences',
	123	+ 'nochange'
	124	+ ),
	125	+ ),
109	126	);
110	127	}
111	128
—	—	@@ -113,7 +130,8 @@
114	131	'title' => 'Title of the page you want to restore.',
115	132	'token' => 'An undelete token previously retrieved through list=deletedrevs',
116	133	'reason' => 'Reason for restoring (optional)',
117		~~- 'timestamps' => 'Timestamps of the revisions to restore. If not set, all revisions will be restored.'~~
	134	+ 'timestamps' => 'Timestamps of the revisions to restore. If not set, all revisions will be restored.',
	135	+ 'watchlist' => 'Unconditionally add or remove the page from your watchlist, use preferences or do not change watch',
118	136	);
119	137	}
120	138
Index: trunk/phase3/includes/api/ApiBase.php
—	—	@@ -532,6 +532,31 @@
533	533
534	534	return $mValidNamespaces;
535	535	}
	536	+ /**
	537	+ * Handle watchlist settings
	538	+ */
	539	+ protected function getWatchlistValue ( $watchlist, $titleObj ) {
	540	+ switch ( $watchlist ) {
	541	+ case 'watch':
	542	+ $watch = true;
	543	+ break;
	544	+ case 'unwatch':
	545	+ $watch = false;
	546	+ break;
	547	+ case 'preferences':
	548	+ global $wgUser;
	549	+
	550	+ if ( $titleObj->exists() ) {
	551	+ $watch = $wgUser->getOption( 'watchdefault' ) \|\| $titleObj->userIsWatching();
	552	+ }
	553	+ break;
	554	+ case 'nochange':
	555	+ default:
	556	+ $watch = $titleObj->userIsWatching();
	557	+ }
	558	+
	559	+ return $watch;
	560	+ }
536	561
537	562	/**
538	563	* Using the settings determine the value for the given parameter
Index: trunk/phase3/includes/api/ApiUpload.php
—	—	@@ -218,10 +218,17 @@
219	219	if ( is_null( $this->mParams['text'] ) ) {
220	220	$this->mParams['text'] = $this->mParams['comment'];
221	221	}
222		-
	222	+
	223	+ $watch = $this->getWatchlistValue( $params['watchlist'] );
	224	+
	225	+ // Deprecated parameters
	226	+ if ( $this->mParams['watch'] ) {
	227	+ $watch = true;
	228	+ }
	229	+
223	230	// No errors, no warnings: do the upload
224	231	$status = $this->mUpload->performUpload( $this->mParams['comment'],
225		~~- $this->mParams['text'], $this->mParams['watch'], $wgUser );~~
	232	+ $this->mParams['text'], $watch, $wgUser );
226	233
227	234	if ( !$status->isGood() ) {
228	235	$error = $status->getErrorsArray();
—	—	@@ -254,7 +261,19 @@
255	262	),
256	263	'text' => null,
257	264	'token' => null,
258		~~- 'watch' => false,~~
	265	+ 'watch' => array(
	266	+ ApiBase::PARAM_DFLT => false,
	267	+ ApiBase::PARAM_DEPRECATED => true,
	268	+ ),
	269	+ 'watchlist' => array(
	270	+ ApiBase::PARAM_DFLT => 'preferences',
	271	+ ApiBase::PARAM_TYPE => array(
	272	+ 'watch',
	273	+ 'unwatch',
	274	+ 'preferences',
	275	+ 'nochange'
	276	+ ),
	277	+ ),
259	278	'ignorewarnings' => false,
260	279	'file' => null,
261	280	'url' => null,
—	—	@@ -270,6 +289,7 @@
271	290	'comment' => 'Upload comment. Also used as the initial page text for new files if "text" is not specified',
272	291	'text' => 'Initial page text for new files',
273	292	'watch' => 'Watch the page',
	293	+ 'watchlist' => 'Unconditionally add or remove the page from your watchlist, use preferences or do not change watch',
274	294	'ignorewarnings' => 'Ignore any warnings',
275	295	'file' => 'File contents',
276	296	'url' => 'Url to fetch the file from',
Index: trunk/phase3/RELEASE-NOTES
—	—	@@ -60,6 +60,7 @@
61	61	== API changes in 1.17 ==
62	62	* (bug 22738) Allow filtering by action type on query=logevent
63	63	* (bug 22764) uselang parameter for action=parse
	64	+* (bug 22944) API: watchlist options are inconsistent
64	65
65	66	=== Languages updated in 1.17 ===
66	67

Follow-up revisions

Revision	Commit summary	Author	Date
r64198	Minor followup to r64197...	reedy	22:31, 25 March 2010
r64267	Followup to r64197...	reedy	17:51, 27 March 2010
r64291	Followup r64197...	reedy	15:08, 28 March 2010
r64403	* Set $titleObj to null by default on getWatchlistValue since it often isn't ...	mah	19:10, 30 March 2010
r64696	Fixup to r64197 per http://www.mediawiki.org/wiki/Special:Code/MediaWiki/6419......	reedy	08:56, 7 April 2010

Past revisions this follows-up on

Revision	Commit summary	Author	Date
r64184	Part of bug 22944...	reedy	20:57, 25 March 2010

Comments

#Comment by Reedy (talk | contribs) 22:21, 25 March 2010

<Reedy> A couple of the if (watch) else if ( !watch) look a bit kooky <Reedy> Hmm. ApiUpload probably won't unwatch.. Do i change performUpload to cater for !watch (possible breaking change?), or just cater for in the API?

#Comment by Gurch (talk | contribs) 23:41, 25 March 2010

Unwatching on upload is rather an obscure case that I can't imagine would get much use, so it's probably not worth breaking changes over. Some of the others were more obviously missing -- e.g. if you watch a page when you protect it, you probably want to unwatch it when you unprotect it :)

Thanks for the changes anyway.

#Comment by Catrope (talk | contribs) 19:35, 26 March 2010

This revision changes a bunch of modules to always call either watch or unwatch on the affected title, even for watchlist=nochange. This is especially bad for move, which calls it on two titles at the same time, leading to a curious side effect (moving a watched title with nochange watches the redirect), and is generally not a good idea if these functions are ever changed to have other effects as well.

#Comment by Catrope (talk | contribs) 19:42, 26 March 2010

+		if ( $watch ) {
+			$articleObj->doWatch();
+		} else if ( !$watch ) {
+			$articleObj->doUnwatch();
+		}

Since these conditions are mutually exclusive, checking for !$watch is redundant.

+		if ( $params['watch'] || $watch ) {
+			$wgUser->addWatch( $titleObj );
+		} else if ( !$watch ) {
+			$wgUser->removeWatch( $titleObj );
+		}

Same here. !( $params['watch'] || $watch ) is equivalent !$params['watch'] && !$watch which implies !$watch.

+			case 'preferences':
+				global $wgUser;
+			
+				if ( $titleObj->exists() ) {
+					$watch = $wgUser->getOption( 'watchdefault' ) || $titleObj->userIsWatching();
+				}
+				break;

This leaves $watch unset when &watchlist=preferences but the page doesn't exist. It should be explicitly set to false in this case, assuming the watchdefault preference doesn't apply to page creations.

#Comment by Catrope (talk | contribs) 19:42, 26 March 2010

+		if ( $watch ) {
+			$articleObj->doWatch();
+		} else if ( !$watch ) {
+			$articleObj->doUnwatch();
+		}

Since these conditions are mutually exclusive, checking for !$watch is redundant.

+		if ( $params['watch'] || $watch ) {
+			$wgUser->addWatch( $titleObj );
+		} else if ( !$watch ) {
+			$wgUser->removeWatch( $titleObj );
+		}

Same here. !( $params['watch'] || $watch ) is equivalent !$params['watch'] && !$watch which implies !$watch.

+			case 'preferences':
+				global $wgUser;
+			
+				if ( $titleObj->exists() ) {
+					$watch = $wgUser->getOption( 'watchdefault' ) || $titleObj->userIsWatching();
+				}
+				break;

#Comment by Reedy (talk | contribs) 23:21, 26 March 2010

I did mention about the mutually exclusive stuff in my first comment ;)

Will get a checkout on my laptop in a bit and look at fixing this up.

As for the "always change"... I agree, but the edit module did do the same. So probably worth tidying that up in the process.

Might need a little bit of thinking about to decide how to improve on this.

#Comment by Catrope (talk | contribs) 19:24, 27 March 2010

No, the edit module didn't do this because it doesn't use $watch to watch/unwatch directly, but passes it to EditPage which in turn has logic to prevent rewatching an already watched page.

#Comment by Reedy (talk | contribs) 23:24, 26 March 2010

But I don't think I've got my ssh key on my laptop yet... So can't actually commit it back anyway

#Comment by Nikerabbit (talk | contribs) 14:58, 27 March 2010

[27-Mar-2010 14:51:16] PHP Notice:  Undefined variable: watch in /www/w/includes/api/ApiBase.php on line 558

Ugh

#Comment by Reedy (talk | contribs) 15:06, 27 March 2010

That'll be the missing else $watch = false from case preferences...

#Comment by MarkAHershberger (talk | contribs) 19:14, 30 March 2010

see r64403 which fixes a problem you created here with UploadBase where you forgot to provide the second parameter.

#Comment by Raymond (talk | contribs) 08:24, 7 April 2010

Seen on translatewiki today:

PHP Notice: Undefined variable: titleObj in /www/w/includes/api/ApiMove.php  on line 126

#Comment by Reedy (talk | contribs) 08:56, 7 April 2010

Mmm, cheers.

Seemingly, just nulling the call would make more sense, as we've got 2 pages, it's

Status & tagging log

09:28, 7 April 2010 Raymond (talk | contribs) changed the status of r64197 [removed: fixme added: resolved]
08:24, 7 April 2010 Raymond (talk | contribs) changed the status of r64197 [removed: resolved added: fixme]
18:25, 28 March 2010 Catrope (talk | contribs) changed the status of r64197 [removed: fixme added: resolved]
19:35, 26 March 2010 Catrope (talk | contribs) changed the status of r64197 [removed: new added: fixme]