r64291 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r64290‎ | r64291 | r64292 >
Date:15:08, 28 March 2010
Author:reedy
Status:ok (Comments)
Tags:
Comment:
Followup r64197

Return null in getWatchlistValue if no change

Fixup unneccessary watch/unwatch calls

Remove useless unwatch from ApiUpload
Modified paths:
  • /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
——@@ -124,15 +124,23 @@
125125
126126 // Watch pages
127127 $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj ) || $wgUser->getOption( 'watchmoves' );
128 -
 128+
129129 // Deprecated parameters
130 - if ( $params['watch'] || $watch ) {
131 - $wgUser->addWatch( $fromTitle );
132 - $wgUser->addWatch( $toTitle );
133 - } elseif ( $params['unwatch'] || !$watch ) {
134 - $wgUser->removeWatch( $fromTitle );
135 - $wgUser->removeWatch( $toTitle );
 130+ if ( $params['watch'] ) {
 131+ $watch = true;
 132+ } elseif ( $params['unwatch'] ) {
 133+ $watch = false;
136134 }
 135+
 136+ if ( $watch !== null ) {
 137+ if ( $watch ) {
 138+ $wgUser->addWatch( $fromTitle );
 139+ $wgUser->addWatch( $toTitle );
 140+ } else {
 141+ $wgUser->removeWatch( $fromTitle );
 142+ $wgUser->removeWatch( $toTitle );
 143+ }
 144+ }
137145 $this->getResult()->addValue( null, $this->getModuleName(), $r );
138146 }
139147
Index: trunk/phase3/includes/api/ApiProtect.php
——@@ -116,11 +116,17 @@
117117
118118 $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj );
119119
120 - if ( $params['watch'] || $watch ) {
121 - $articleObj->doWatch();
122 - } else {
123 - $articleObj->doUnwatch();
 120+ if ( $params['watch'] ) {
 121+ $watch = true;
124122 }
 123+
 124+ if ( $watch !== null ) {
 125+ if ( $watch ) {
 126+ $articleObj->doWatch();
 127+ } else {
 128+ $articleObj->doUnwatch();
 129+ }
 130+ }
125131
126132 if ( $titleObj->exists() ) {
127133 $ok = $articleObj->updateRestrictions( $protections, $params['reason'], $cascade, $expiryarray );
Index: trunk/phase3/includes/api/ApiRollback.php
——@@ -75,10 +75,12 @@
7676
7777 $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj );
7878
79 - if ( $watch ) {
80 - $articleObj->doWatch();
81 - } else if ( !$watch ) {
82 - $articleObj->doUnwatch();
 79+ if ( $watch !== null) {
 80+ if ( $watch ) {
 81+ $articleObj->doWatch();
 82+ } else {
 83+ $articleObj->doUnwatch();
 84+ }
8385 }
8486
8587 $info = array(
Index: trunk/phase3/includes/api/ApiDelete.php
——@@ -85,11 +85,19 @@
8686 $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj ) || $wgUser->getOption( 'watchdeletion' );
8787
8888 // Deprecated parameters
89 - if ( $params['watch'] || $watch ) {
90 - $articleObj->doWatch();
91 - } elseif ( $params['unwatch'] || !$watch ) {
92 - $articleObj->doUnwatch();
 89+ if ( $params['watch'] ) {
 90+ $watch = true;
 91+ } elseif ( $params['unwatch'] ) {
 92+ $watch = false;
9393 }
 94+
 95+ if ( $watch !== null ) {
 96+ if ( $watch ) {
 97+ $articleObj->doWatch();
 98+ } else {
 99+ $articleObj->doUnwatch();
 100+ }
 101+ }
94102 }
95103
96104 $r = array( 'title' => $titleObj->getPrefixedText(), 'reason' => $reason );
Index: trunk/phase3/includes/api/ApiEditPage.php
——@@ -211,7 +211,7 @@
212212 $watch = false;
213213 }
214214
215 - if ( $watch ) {
 215+ if ( $watch || $titleObj->userIsWatching() ) {
216216 $reqArr['wpWatchthis'] = '';
217217 }
218218
Index: trunk/phase3/includes/api/ApiUndelete.php
——@@ -84,10 +84,12 @@
8585
8686 $watch = $this->getWatchlistValue( $params['watchlist'], $titleObj );
8787
88 - if ( $params['watch'] || $watch ) {
89 - $wgUser->addWatch( $titleObj );
90 - } else {
91 - $wgUser->removeWatch( $titleObj );
 88+ if ( $watch !== null ) {
 89+ if ( $watch ) {
 90+ $wgUser->addWatch( $titleObj );
 91+ } else {
 92+ $wgUser->removeWatch( $titleObj );
 93+ }
9294 }
9395
9496 $info['title'] = $titleObj->getPrefixedText();
Index: trunk/phase3/includes/api/ApiBase.php
——@@ -542,14 +542,14 @@
543543 case 'unwatch':
544544 return false;
545545 case 'preferences':
546 - if ( $titleObj->exists() ) {
547 - global $wgUser;
548 - return $wgUser->getOption( 'watchdefault' ) || $titleObj->userIsWatching();
 546+ global $wgUser;
 547+ if ( $titleObj->exists() && !$titleObj->userIsWatching() && $wgUser->getOption( 'watchdefault' ) ) {
 548+ return true;
549549 }
550 - return false;
 550+ return null;
551551 case 'nochange':
552552 default:
553 - return $titleObj->userIsWatching();
 553+ return null;
554554 }
555555 }
556556
Index: trunk/phase3/includes/api/ApiUpload.php
——@@ -239,10 +239,6 @@
240240
241241 $file = $this->mUpload->getLocalFile();
242242
243 - if ( !$watch ) {
244 - $wgUser->removeWatch( $file->getTitle() );
245 - }
246 -
247243 $result['result'] = 'Success';
248244 $result['filename'] = $file->getName();
249245 $result['imageinfo'] = $this->mUpload->getImageInfo( $this->getResult() );
——@@ -274,7 +270,6 @@
275271 ApiBase::PARAM_DFLT => 'preferences',
276272 ApiBase::PARAM_TYPE => array(
277273 'watch',
278 - 'unwatch',
279274 'preferences',
280275 'nochange'
281276 ),

Follow-up revisions

RevisionCommit summaryAuthorDate
r64302Switch if ordering as per Roans comment for r64291reedy19:23, 28 March 2010
r64852* Clean up some duplicated code in r64291...mah06:11, 10 April 2010
r64962re r64291 and r64852 use ApiBase::setWatch instead of wgUser->*Watch to set w...mah15:44, 12 April 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r64197Fix bug 22944 in a much better fashion (using watchlist parameter)...reedy22:15, 25 March 2010

Comments

#Comment by Catrope (talk | contribs)   16:10, 28 March 2010 
+				if ( $titleObj->exists() && !$titleObj->userIsWatching() && $wgUser->getOption( 'watchdefault' ) ) {

I suggest reversing the order of the userIsWatching() and getOption() calls for performance (the latter doesn't need an extra DB query), and removing the trailing tabs on this line.

Status & tagging log