r86044 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r86043‎ | r86044 | r86045 >
Date:12:17, 14 April 2011
Author:happy-melon
Status:ok (Comments)
Tags:
Comment:
Follow-up r 86041 per CR and IRC:
* Article constructor needs to be called with zero as second parameter
* Run stylize.php over new files
* Add Action::getLang() for consistency with other context accessors
* Fix declaration of FormAction::alterForm(), doesn't need to be passed by reference
* Fix inline use of Credits::getCredits() in SkinTemplate and SkinLegacy
Modified paths:
  • /trunk/extensions/ReplaceText/ReplaceTextJob.php (modified) (history)
  • /trunk/phase3/includes/Action.php (modified) (history)
  • /trunk/phase3/includes/SkinLegacy.php (modified) (history)
  • /trunk/phase3/includes/SkinTemplate.php (modified) (history)
  • /trunk/phase3/includes/actions/CreditsAction.php (modified) (history)
  • /trunk/phase3/includes/actions/PurgeAction.php (modified) (history)
  • /trunk/phase3/includes/actions/WatchAction.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/actions/CreditsAction.php
——@@ -25,11 +25,11 @@
2626
2727 class CreditsAction extends FormlessAction {
2828
29 - public function getName(){
 29+ public function getName() {
3030 return 'credits';
3131 }
3232
33 - public function getRestriction(){
 33+ public function getRestriction() {
3434 return null;
3535 }
3636
——@@ -57,7 +57,7 @@
5858 * @param $showIfMax Bool: whether to contributors if there more than $cnt
5959 * @return String: html
6060 */
61 - protected function getCredits( $cnt, $showIfMax = true ) {
 61+ public function getCredits( $cnt, $showIfMax = true ) {
6262 wfProfileIn( __METHOD__ );
6363 $s = '';
6464
Index: trunk/phase3/includes/actions/PurgeAction.php
——@@ -25,19 +25,19 @@
2626
2727 class PurgeAction extends FormAction {
2828
29 - public function getName(){
 29+ public function getName() {
3030 return 'purge';
3131 }
3232
33 - public function getRestriction(){
 33+ public function getRestriction() {
3434 return null;
3535 }
3636
37 - public function requiresUnblock(){
 37+ public function requiresUnblock() {
3838 return false;
3939 }
4040
41 - public function getDescription(){
 41+ public function getDescription() {
4242 return '';
4343 }
4444
——@@ -45,11 +45,11 @@
4646 * Just get an empty form with a single submit button
4747 * @return array
4848 */
49 - protected function getFormFields(){
 49+ protected function getFormFields() {
5050 return array();
5151 }
5252
53 - public function onSubmit( $data ){
 53+ public function onSubmit( $data ) {
5454 $this->page->doPurge();
5555 return true;
5656 }
——@@ -58,36 +58,36 @@
5959 * purge is slightly wierd because it can be either formed or formless depending
6060 * on user permissions
6161 */
62 - public function show(){
 62+ public function show() {
6363 $this->setHeaders();
6464
6565 // This will throw exceptions if there's a problem
6666 $this->checkCanExecute( $this->getUser() );
6767
68 - if( $this->getUser()->isAllowed( 'purge' ) ){
 68+ if ( $this->getUser()->isAllowed( 'purge' ) ) {
6969 $this->onSubmit( array() );
7070 $this->onSuccess();
7171 } else {
7272 $form = $this->getForm();
73 - if( $form->show() ){
 73+ if ( $form->show() ) {
7474 $this->onSuccess();
7575 }
7676 }
7777 }
7878
79 - protected function alterForm( HTMLForm $form ){
 79+ protected function alterForm( HTMLForm $form ) {
8080 $form->setSubmitText( wfMsg( 'confirm_purge_button' ) );
8181 }
8282
83 - protected function preText(){
 83+ protected function preText() {
8484 return wfMessage( 'confirm-purge-top' )->parse();
8585 }
8686
87 - protected function postText(){
 87+ protected function postText() {
8888 return wfMessage( 'confirm-purge-bottom' )->parse();
8989 }
9090
91 - public function onSuccess(){
 91+ public function onSuccess() {
9292 $this->getOutput()->redirect( $this->getTitle() );
9393 }
9494 }
Index: trunk/phase3/includes/actions/WatchAction.php
——@@ -22,23 +22,23 @@
2323
2424 class WatchAction extends FormlessAction {
2525
26 - public function getName(){
 26+ public function getName() {
2727 return 'watch';
2828 }
2929
30 - public function getRestriction(){
 30+ public function getRestriction() {
3131 return 'read';
3232 }
3333
34 - public function requiresUnblock(){
 34+ public function requiresUnblock() {
3535 return false;
3636 }
3737
38 - protected function getDescription(){
 38+ protected function getDescription() {
3939 return wfMsg( 'addedwatch' );
4040 }
4141
42 - protected function checkCanExecute( User $user ){
 42+ protected function checkCanExecute( User $user ) {
4343 if ( $user->isAnon() ) {
4444 throw new ErrorPageError( 'watchnologin', 'watchnologintext' );
4545 }
——@@ -62,11 +62,11 @@
6363
6464 class UnwatchAction extends WatchAction {
6565
66 - public function getName(){
 66+ public function getName() {
6767 return 'unwatch';
6868 }
6969
70 - protected function getDescription(){
 70+ protected function getDescription() {
7171 return wfMsg( 'removedwatch' );
7272 }
7373
Index: trunk/phase3/includes/SkinLegacy.php
——@@ -210,7 +210,7 @@
211211 }
212212
213213 if ( $wgMaxCredits != 0 ) {
214 - $s .= ' ' . Credits::getCredits( $article, $wgMaxCredits, $wgShowCreditsIfMax );
 214+ $s .= ' ' . Action::factory( 'credits', $article )->getCredits( $wgMaxCredits, $wgShowCreditsIfMax );
215215 } else {
216216 $s .= $this->data['lastmod'];
217217 }
Index: trunk/phase3/includes/SkinTemplate.php
——@@ -382,7 +382,7 @@
383383 $this->credits = false;
384384
385385 if( $wgMaxCredits != 0 ){
386 - $this->credits = Credits::getCredits( $article, $wgMaxCredits, $wgShowCreditsIfMax );
 386+ $this->credits = Action::factory( 'credits', $article )->getCredits( $wgMaxCredits, $wgShowCreditsIfMax );
387387 } else {
388388 $tpl->set( 'lastmod', $this->lastModified( $article ) );
389389 }
Index: trunk/phase3/includes/Action.php
——@@ -43,19 +43,19 @@
4444 * @param $action String
4545 * @return bool|null|string
4646 */
47 - private final static function getClass( $action ){
 47+ private final static function getClass( $action ) {
4848 global $wgActions;
4949 $action = strtolower( $action );
5050
51 - if( !isset( $wgActions[$action] ) ){
 51+ if ( !isset( $wgActions[$action] ) ) {
5252 return null;
5353 }
5454
55 - if( $wgActions[$action] === false ){
 55+ if ( $wgActions[$action] === false ) {
5656 return false;
5757 }
5858
59 - elseif( $wgActions[$action] === true ){
 59+ elseif ( $wgActions[$action] === true ) {
6060 return ucfirst( $action ) . 'Action';
6161 }
6262
——@@ -71,9 +71,9 @@
7272 * @return Action|false|null false if the action is disabled, null
7373 * if it is not recognised
7474 */
75 - public final static function factory( $action, Article $page ){
 75+ public final static function factory( $action, Article $page ) {
7676 $class = self::getClass( $action );
77 - if( $class ){
 77+ if ( $class ) {
7878 $obj = new $class( $page );
7979 return $obj;
8080 }
——@@ -94,8 +94,8 @@
9595 * Get the RequestContext in use here
9696 * @return RequestContext
9797 */
98 - protected final function getContext(){
99 - if( $this->context instanceof RequestContext ){
 98+ protected final function getContext() {
 99+ if ( $this->context instanceof RequestContext ) {
100100 return $this->context;
101101 }
102102 return $this->page->getContext();
——@@ -138,10 +138,19 @@
139139 }
140140
141141 /**
 142+ * Shortcut to get the user Language being used for this instance
 143+ *
 144+ * @return Skin
 145+ */
 146+ protected final function getLang() {
 147+ return $this->getContext()->lang;
 148+ }
 149+
 150+ /**
142151 * Shortcut to get the Title object from the page
143152 * @return Title
144153 */
145 - protected final function getTitle(){
 154+ protected final function getTitle() {
146155 return $this->page->getTitle();
147156 }
148157
——@@ -150,7 +159,7 @@
151160 * these things in the real world
152161 * @param Article $page
153162 */
154 - protected function __construct( Article $page ){
 163+ protected function __construct( Article $page ) {
155164 $this->page = $page;
156165 }
157166
——@@ -175,15 +184,15 @@
176185 * @throws ErrorPageError
177186 */
178187 protected function checkCanExecute( User $user ) {
179 - if( $this->requiresWrite() && wfReadOnly() ){
 188+ if ( $this->requiresWrite() && wfReadOnly() ) {
180189 throw new ReadOnlyError();
181190 }
182191
183 - if( $this->getRestriction() !== null && !$user->isAllowed( $this->getRestriction() ) ){
 192+ if ( $this->getRestriction() !== null && !$user->isAllowed( $this->getRestriction() ) ) {
184193 throw new PermissionsError( $this->getRestriction() );
185194 }
186195
187 - if( $this->requiresUnblock() && $user->isBlocked() ){
 196+ if ( $this->requiresUnblock() && $user->isBlocked() ) {
188197 $block = $user->mBlock;
189198 throw new UserBlockedError( $block );
190199 }
——@@ -193,7 +202,7 @@
194203 * Whether this action requires the wiki not to be locked
195204 * @return Bool
196205 */
197 - public function requiresWrite(){
 206+ public function requiresWrite() {
198207 return true;
199208 }
200209
——@@ -201,7 +210,7 @@
202211 * Whether this action can still be executed by a blocked user
203212 * @return Bool
204213 */
205 - public function requiresUnblock(){
 214+ public function requiresUnblock() {
206215 return true;
207216 }
208217
——@@ -220,10 +229,6 @@
221230 /**
222231 * Returns the name that goes in the \<h1\> page title
223232 *
224 - * Derived classes can override this, but usually it is easier to keep the
225 - * default behaviour. Messages can be added at run-time, see
226 - * MessageCache.php.
227 - *
228233 * @return String
229234 */
230235 protected function getDescription() {
——@@ -259,20 +264,20 @@
260265 * Add pre- or post-text to the form
261266 * @return String HTML which will be sent to $form->addPreText()
262267 */
263 - protected function preText(){ return ''; }
264 - protected function postText(){ return ''; }
 268+ protected function preText() { return ''; }
 269+ protected function postText() { return ''; }
265270
266271 /**
267272 * Play with the HTMLForm if you need to more substantially
268 - * @param &$form HTMLForm
 273+ * @param $form HTMLForm
269274 */
270 - protected function alterForm( HTMLForm &$form ){}
 275+ protected function alterForm( HTMLForm $form ) {}
271276
272277 /**
273278 * Get the HTMLForm to control behaviour
274279 * @return HTMLForm|null
275280 */
276 - protected function getForm(){
 281+ protected function getForm() {
277282 $this->fields = $this->getFormFields();
278283
279284 // Give hooks a chance to alter the form, adding extra fields or text etc
——@@ -315,14 +320,14 @@
316321 * display something new or redirect to somewhere. Some actions have more exotic
317322 * behaviour, but that's what subclassing is for :D
318323 */
319 - public function show(){
 324+ public function show() {
320325 $this->setHeaders();
321326
322327 // This will throw exceptions if there's a problem
323328 $this->checkCanExecute( $this->getUser() );
324329
325330 $form = $this->getForm();
326 - if( $form->show() ){
 331+ if ( $form->show() ) {
327332 $this->onSuccess();
328333 }
329334 }
——@@ -334,7 +339,7 @@
335340 * @param bool $captureErrors
336341 * @return bool
337342 */
338 - public function execute( array $data = null, $captureErrors = true ){
 343+ public function execute( array $data = null, $captureErrors = true ) {
339344 try {
340345 // Set a new context so output doesn't leak.
341346 $this->context = clone $this->page->getContext();
——@@ -343,17 +348,17 @@
344349 $this->checkCanExecute( $this->getUser() );
345350
346351 $fields = array();
347 - foreach( $this->fields as $key => $params ){
348 - if( isset( $data[$key] ) ){
 352+ foreach ( $this->fields as $key => $params ) {
 353+ if ( isset( $data[$key] ) ) {
349354 $fields[$key] = $data[$key];
350 - } elseif( isset( $params['default'] ) ) {
 355+ } elseif ( isset( $params['default'] ) ) {
351356 $fields[$key] = $params['default'];
352357 } else {
353358 $fields[$key] = null;
354359 }
355360 }
356361 $status = $this->onSubmit( $fields );
357 - if( $status === true ){
 362+ if ( $status === true ) {
358363 // This might do permanent stuff
359364 $this->onSuccess();
360365 return true;
——@@ -361,8 +366,8 @@
362367 return false;
363368 }
364369 }
365 - catch ( ErrorPageError $e ){
366 - if( $captureErrors ){
 370+ catch ( ErrorPageError $e ) {
 371+ if ( $captureErrors ) {
367372 return false;
368373 } else {
369374 throw $e;
——@@ -388,19 +393,19 @@
389394 /**
390395 * We don't want an HTMLForm
391396 */
392 - protected function getFormFields(){
 397+ protected function getFormFields() {
393398 return false;
394399 }
395400
396 - public function onSubmit( $data ){
 401+ public function onSubmit( $data ) {
397402 return false;
398403 }
399404
400 - public function onSuccess(){
 405+ public function onSuccess() {
401406 return false;
402407 }
403408
404 - public function show(){
 409+ public function show() {
405410 $this->setHeaders();
406411
407412 // This will throw exceptions if there's a problem
——@@ -416,11 +421,11 @@
417422 * @param $captureErrors Bool whether to catch exceptions and just return false
418423 * @return Bool whether execution was successful
419424 */
420 - public function execute( array $data = null, $captureErrors = true){
 425+ public function execute( array $data = null, $captureErrors = true ) {
421426 try {
422427 // Set a new context so output doesn't leak.
423428 $this->context = clone $this->page->getContext();
424 - if( is_array( $data ) ){
 429+ if ( is_array( $data ) ) {
425430 $this->context->setRequest( new FauxRequest( $data, false ) );
426431 }
427432
——@@ -430,8 +435,8 @@
431436 $this->onView();
432437 return true;
433438 }
434 - catch ( ErrorPageError $e ){
435 - if( $captureErrors ){
 439+ catch ( ErrorPageError $e ) {
 440+ if ( $captureErrors ) {
436441 return false;
437442 } else {
438443 throw $e;
Index: trunk/extensions/ReplaceText/ReplaceTextJob.php
——@@ -42,11 +42,11 @@
4343 $create_redirect = $this->params['create_redirect'];
4444 $this->title->moveTo( $new_title, true, $reason, $create_redirect );
4545 if ( $this->params['watch_page'] ) {
46 - Action::factory( 'watch', new Article( $new_title ) )->execute();
 46+ Action::factory( 'watch', new Article( $new_title, 0 ) )->execute();
4747 }
4848 $wgUser = $actual_user;
4949 } else {
50 - $article = new Article( $this->title );
 50+ $article = new Article( $this->title, 0 );
5151 if ( !$article ) {
5252 $this->error = 'replaceText: Article not found "' . $this->title->getPrefixedDBkey() . '"';
5353 wfProfileOut( __METHOD__ );

Sign-offs

UserFlagDate
Krinkleinspected09:17, 6 June 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r90877Follow-up r86044 CR (correct subtitle message for action=credits), and some d...happy-melon14:05, 27 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r86041r86001, now with less scariness :P I took out the delete action and did purg...happy-melon10:38, 14 April 2011

Comments

#Comment by Bawolff (talk | contribs)   22:46, 8 June 2011

Not sure if this is right revision (but can't find the right one), but for the CreditsAction: The message for the subtitle should be mediawiki:creditspage, somewhere along the way it got changed to mediawikis:credits

#Comment by Happy-melon (talk | contribs)   14:06, 27 June 2011

Fixed in r90877.

Status & tagging log