r85994 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r85993‎ | r85994 | r85995 >
Date:22:30, 13 April 2011
Author:happy-melon
Status:resolved (Comments)
Tags:
Comment:
Unbundle OutputPage::showErrorPage() in Exception.php to allow ErrorPageError objects to be passed a Message object for a more complicated display.
Modified paths:
  • /trunk/phase3/includes/Exception.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Exception.php
——@@ -324,13 +324,35 @@
325325 $this->title = $title;
326326 $this->msg = $msg;
327327 $this->params = $params;
328 - parent::__construct( wfMsg( $msg ) );
 328+
 329+ if( $msg instanceof Message ){
 330+ parent::__construct( $msg );
 331+ } else {
 332+ parent::__construct( wfMsg( $msg ) );
 333+ }
329334 }
330335
331336 function report() {
332337 global $wgOut;
333338
334 - $wgOut->showErrorPage( $this->title, $this->msg, $this->params );
 339+ if ( $wgOut->getTitle() ) {
 340+ $wgOut->debug( 'Original title: ' . $wgOut->getTitle()->getPrefixedText() . "\n" );
 341+ }
 342+ $wgOut->setPageTitle( wfMsg( $this->title ) );
 343+ $wgOut->setHTMLTitle( wfMsg( 'errorpagetitle' ) );
 344+ $wgOut->setRobotPolicy( 'noindex,nofollow' );
 345+ $wgOut->setArticleRelated( false );
 346+ $wgOut->enableClientCache( false );
 347+ $wgOut->mRedirect = '';
 348+ $wgOut->clearHTML();
 349+
 350+ if( $this->msg instanceof Message ){
 351+ $wgOut->addHTML( $this->msg->parse() );
 352+ } else {
 353+ $wgOut->addWikiMsgArray( $this->msg, $this->params );
 354+ }
 355+
 356+ $wgOut->returnToMain();
335357 $wgOut->output();
336358 }
337359 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r95470Followup r85994: eliminate code duplication introduced by this revision, inst...catrope10:13, 25 August 2011

Comments

#Comment by Nikerabbit (talk | contribs)   07:20, 14 April 2011

Should probably be 

+$wgOut->addHTML( $this->msg->parseBlock() );

But! I think better idea would be to modify (add|wrap)WikiMsg(Array)? to accept Message objects. That would be more natural. Also you could construct a message object from the message key and params, so you don't need to check later which one you got.

#Comment by Werdna (talk | contribs)   17:37, 15 July 2011

+ if( $msg instanceof Message ){ + parent::__construct( $msg ); + } else { + parent::__construct( wfMsg( $msg ) ); + }

Should this be reversed?

#Comment by Happy-melon (talk | contribs)   23:41, 15 July 2011

Don't think so: the parent constructor expects message text (or something it can pretend to be text), so if it's already a Message object pass that straight through, otherwise assume it's a message key and get the text via wfMsg() as before.

#Comment by Happy-melon (talk | contribs)   20:02, 26 July 2011

I believe this is correct. Resetting to new.

#Comment by Werdna (talk | contribs)   17:40, 15 July 2011

Also, I wonder if you'd be better off modifying showErrorPage to accept Message objects instead of copying and modifying it inline.

#Comment by Happy-melon (talk | contribs)   23:45, 15 July 2011

I think the other way: showErrorPage() should be deprecated; the logical place to put code for displaying an Exception is with the code for the Exception.

#Comment by Catrope (talk | contribs)   10:01, 11 August 2011

Has either of these things been done yet? Right now this revision just introduces duplicated code.

#Comment by Catrope (talk | contribs)   10:04, 25 August 2011

Ping, it's been two weeks and no answer. I'm setting this back to fixme because of the code duplication.

#Comment by Catrope (talk | contribs)   10:14, 25 August 2011

Meh, never mind, eliminating the code duplication was easy. Done in r95470.

Status & tagging log