r77714 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r77713‎ | r77714 | r77715 >
Date:13:27, 4 December 2010
Author:ialex
Status:ok (Comments)
Tags:
Comment:
* Converted UserMailer stuff to return a Status object instead of true-or-WikiError
* Made WikiError::isError() compatible with Status objects
* Added Status::getMessage() for backward compatibility

Extensions using WikiError::isError() to detect a failure of UserMailer::send() and realted methods should still work like before
Modified paths:
  • /trunk/phase3/includes/Preferences.php (modified) (history)
  • /trunk/phase3/includes/Status.php (modified) (history)
  • /trunk/phase3/includes/User.php (modified) (history)
  • /trunk/phase3/includes/UserMailer.php (modified) (history)
  • /trunk/phase3/includes/WikiError.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialConfirmemail.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialEmailuser.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialUserlogin.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/language/messageTypes.inc (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/language/messages.inc
——@@ -463,6 +463,11 @@
464464 'loginlanguagelinks',
465465 'suspicious-userlogout',
466466 ),
 467+ 'mail' => array(
 468+ 'pear-mail-error',
 469+ 'php-mail-error',
 470+ 'php-mail-error-unknown',
 471+ ),
467472 'passwordstrength' => array(
468473 'password-strength',
469474 'password-strength-bad',
——@@ -3268,6 +3273,7 @@
32693274 'errors' => 'General errors',
32703275 'virus' => 'Virus scanner',
32713276 'login' => 'Login and logout pages',
 3277+ 'mail' => 'E-mail sending',
32723278 'passwordstrength' => 'JavaScript password checks',
32733279 'resetpass' => 'Password reset dialog',
32743280 'toolbar' => 'Edit page toolbar',
Index: trunk/phase3/maintenance/language/messageTypes.inc
——@@ -91,6 +91,8 @@
9292 'loginstart',
9393 'loginend',
9494 'loginlanguagelinks',
 95+ 'pear-mail-error',
 96+ 'php-mail-error',
9597 'markaspatrolledlink',
9698 'newarticletextanon',
9799 'newsectionheaderdefaultlevel',
Index: trunk/phase3/includes/User.php
——@@ -2893,7 +2893,7 @@
28942894 * mail to the user's given address.
28952895 *
28962896 * @param $changed Boolean: whether the adress changed
2897 - * @return \types{\bool,\type{WikiError}} True on success, a WikiError object on failure.
 2897+ * @return Status object
28982898 */
28992899 function sendConfirmationMail( $changed = false ) {
29002900 global $wgLang;
——@@ -2923,7 +2923,7 @@
29242924 * @param $body \string Message body
29252925 * @param $from \string Optional From address; if unspecified, default $wgPasswordSender will be used
29262926 * @param $replyto \string Reply-To address
2927 - * @return \types{\bool,\type{WikiError}} True on success, a WikiError object on failure
 2927+ * @return Status object
29282928 */
29292929 function sendMail( $subject, $body, $from = null, $replyto = null ) {
29302930 if( is_null( $from ) ) {
Index: trunk/phase3/includes/Status.php
——@@ -299,4 +299,13 @@
300300 }
301301 return false;
302302 }
 303+
 304+ /**
 305+ * Backward compatibility function for WikiError -> Status migration
 306+ *
 307+ * @return String
 308+ */
 309+ public function getMessage() {
 310+ return $this->getWikiText();
 311+ }
303312 }
Index: trunk/phase3/includes/UserMailer.php
——@@ -90,9 +90,9 @@
9191 # Based on the result return an error string,
9292 if( PEAR::isError( $mailResult ) ) {
9393 wfDebug( "PEAR::Mail failed: " . $mailResult->getMessage() . "\n" );
94 - return new WikiError( $mailResult->getMessage() );
 94+ return Status::newFatal( 'pear-mail-error', $mailResult->getMessage() );
9595 } else {
96 - return true;
 96+ return Status::newGood();
9797 }
9898 }
9999
——@@ -108,7 +108,7 @@
109109 * @param $body String: email's text.
110110 * @param $replyto MailAddress: optional reply-to email (default: null).
111111 * @param $contentType String: optional custom Content-Type
112 - * @return mixed True on success, a WikiError object on failure.
 112+ * @return Status object
113113 */
114114 static function send( $to, $from, $subject, $body, $replyto=null, $contentType=null ) {
115115 global $wgSMTP, $wgOutputEncoding, $wgEnotifImpersonal;
——@@ -179,19 +179,20 @@
180180 if( PEAR::isError( $mail_object ) ) {
181181 wfDebug( "PEAR::Mail factory failed: " . $mail_object->getMessage() . "\n" );
182182 wfRestoreWarnings();
183 - return new WikiError( $mail_object->getMessage() );
 183+ return Status::newFatal( 'pear-mail-error', $mail_object->getMessage() );
184184 }
185185
186186 wfDebug( "Sending mail via PEAR::Mail to $dest\n" );
187187 $chunks = array_chunk( (array)$dest, $wgEnotifMaxRecips );
188188 foreach ($chunks as $chunk) {
189 - $e = self::sendWithPear($mail_object, $chunk, $headers, $body);
190 - if( WikiError::isError( $e ) ) {
 189+ $status = self::sendWithPear($mail_object, $chunk, $headers, $body);
 190+ if( !$status->isOK() ) {
191191 wfRestoreWarnings();
192 - return $e;
 192+ return $status;
193193 }
194194 }
195195 wfRestoreWarnings();
 196+ return Status::newGood();
196197 } else {
197198 # In the following $headers = expression we removed "Reply-To: {$from}\r\n" , because it is treated differently
198199 # (fifth parameter of the PHP mail function, see some lines below)
——@@ -236,13 +237,13 @@
237238
238239 if ( self::$mErrorString ) {
239240 wfDebug( "Error sending mail: " . self::$mErrorString . "\n" );
240 - return new WikiError( self::$mErrorString );
 241+ return Status::newFatal( 'php-mail-error', self::$mErrorString );
241242 } elseif (! $sent ) {
242243 //mail function only tells if there's an error
243244 wfDebug( "Error sending mail\n" );
244 - return new WikiError( 'mail() failed' );
 245+ return Status::newFatal( 'php-mail-error-unknown' );
245246 } else {
246 - return true;
 247+ return Status::newGood();
247248 }
248249 }
249250 }
Index: trunk/phase3/includes/specials/SpecialUserlogin.php
——@@ -170,8 +170,8 @@
171171 $wgOut->setRobotPolicy( 'noindex,nofollow' );
172172 $wgOut->setArticleRelated( false );
173173
174 - if( WikiError::isError( $result ) ) {
175 - $this->mainLoginForm( wfMsg( 'mailerror', $result->getMessage() ) );
 174+ if( !$result->isGood() ) {
 175+ $this->mainLoginForm( wfMsg( 'mailerror', $result->getWikiText() ) );
176176 } else {
177177 $wgOut->addWikiMsg( 'accmailtext', $u->getName(), $u->getEmail() );
178178 $wgOut->returnToMain( false );
——@@ -199,11 +199,11 @@
200200
201201 # Send out an email authentication message if needed
202202 if( $wgEmailAuthentication && User::isValidEmailAddr( $u->getEmail() ) ) {
203 - $error = $u->sendConfirmationMail();
204 - if( WikiError::isError( $error ) ) {
205 - $wgOut->addWikiMsg( 'confirmemail_sendfailed', $error->getMessage() );
 203+ $status = $u->sendConfirmationMail();
 204+ if( $status->isGood() ) {
 205+ $wgOut->addWikiMsg( 'confirmemail_oncreate' );
206206 } else {
207 - $wgOut->addWikiMsg( 'confirmemail_oncreate' );
 207+ $wgOut->addWikiText( $status->getWikiText( 'confirmemail_sendfailed' ) );
208208 }
209209 }
210210
——@@ -787,11 +787,11 @@
788788 }
789789
790790 $result = $this->mailPasswordInternal( $u, true, 'passwordremindertitle', 'passwordremindertext' );
791 - if( WikiError::isError( $result ) ) {
792 - $this->mainLoginForm( wfMsg( 'mailerror', $result->getMessage() ) );
793 - } else {
 791+ if( $result->isGood() ) {
794792 $this->mainLoginForm( wfMsg( 'passwordsent', $u->getName() ), 'success' );
795793 self::clearLoginToken();
 794+ } else {
 795+ $this->mainLoginForm( $result->getWikiText( 'mailerror' ) );
796796 }
797797 }
798798
——@@ -801,18 +801,18 @@
802802 * @param $throttle Boolean
803803 * @param $emailTitle String: message name of email title
804804 * @param $emailText String: message name of email text
805 - * @return Mixed: true on success, WikiError on failure
 805+ * @return Status object
806806 * @private
807807 */
808808 function mailPasswordInternal( $u, $throttle = true, $emailTitle = 'passwordremindertitle', $emailText = 'passwordremindertext' ) {
809809 global $wgServer, $wgScript, $wgUser, $wgNewPasswordExpiry;
810810
811811 if ( $u->getEmail() == '' ) {
812 - return new WikiError( wfMsg( 'noemail', $u->getName() ) );
 812+ return Status::newFatal( 'noemail', $u->getName() );
813813 }
814814 $ip = wfGetIP();
815815 if( !$ip ) {
816 - return new WikiError( wfMsg( 'badipaddress' ) );
 816+ return Status::newFatal( 'badipaddress' );
817817 }
818818
819819 wfRunHooks( 'User::mailPasswordInternal', array( &$wgUser, &$ip, &$u ) );
Index: trunk/phase3/includes/specials/SpecialConfirmemail.php
——@@ -81,11 +81,11 @@
8282 function showRequestForm() {
8383 global $wgOut, $wgUser, $wgLang, $wgRequest;
8484 if( $wgRequest->wasPosted() && $wgUser->matchEditToken( $wgRequest->getText( 'token' ) ) ) {
85 - $ok = $wgUser->sendConfirmationMail();
86 - if ( WikiError::isError( $ok ) ) {
87 - $wgOut->addWikiMsg( 'confirmemail_sendfailed', $ok->toString() );
 85+ $status = $wgUser->sendConfirmationMail();
 86+ if ( $status->isGood() ) {
 87+ $wgOut->addWikiMsg( 'confirmemail_sent' );
8888 } else {
89 - $wgOut->addWikiMsg( 'confirmemail_sent' );
 89+ $wgOut->addWikiText( $status->getWikiText( 'confirmemail_sendfailed' ) );
9090 }
9191 } else {
9292 if( $wgUser->isEmailConfirmed() ) {
Index: trunk/phase3/includes/specials/SpecialEmailuser.php
——@@ -142,7 +142,7 @@
143143 $wgOut->setPagetitle( wfMsg( 'emailpage' ) );
144144 $result = $form->show();
145145
146 - if( $result === true ){
 146+ if( $result === true || ( $result instanceof Status && $result->isGood() ) ){
147147 $wgOut->setPagetitle( wfMsg( 'emailsent' ) );
148148 $wgOut->addWikiMsg( 'emailsenttext' );
149149 $wgOut->returnToMain( false, $this->mTargetObj->getUserPage() );
——@@ -277,10 +277,10 @@
278278 $replyTo = null;
279279 }
280280
281 - $mailResult = UserMailer::send( $to, $mailFrom, $subject, $text, $replyTo );
 281+ $status = UserMailer::send( $to, $mailFrom, $subject, $text, $replyTo );
282282
283 - if( WikiError::isError( $mailResult ) && false ) {
284 - return $mailResult->getMessage();
 283+ if( !$status->isGood() && false ) {
 284+ return $status;
285285 } else {
286286 // if the user requested a copy of this mail, do this now,
287287 // unless they are emailing themselves, in which case one
——@@ -292,20 +292,12 @@
293293 $subject
294294 );
295295 wfRunHooks( 'EmailUserCC', array( &$from, &$from, &$cc_subject, &$text ) );
296 - $ccResult = UserMailer::send( $from, $from, $cc_subject, $text );
297 - if( WikiError::isError( $ccResult ) ) {
298 - // At this stage, the user's CC mail has failed, but their
299 - // original mail has succeeded. It's unlikely, but still,
300 - // what to do? We can either show them an error, or we can
301 - // say everything was fine, or we can say we sort of failed
302 - // AND sort of succeeded. Of these options, simply saying
303 - // there was an error is probably best.
304 - return $ccResult->getMessage();
305 - }
 296+ $ccStatus = UserMailer::send( $from, $from, $cc_subject, $text );
 297+ $status->merge( $ccStatus );
306298 }
307299
308300 wfRunHooks( 'EmailUserComplete', array( $to, $from, $subject, $text ) );
309 - return true;
 301+ return $status;
310302 }
311303 }
312304 }
Index: trunk/phase3/includes/WikiError.php
——@@ -60,7 +60,13 @@
6161 * @return bool
6262 */
6363 public static function isError( $object ) {
64 - return $object instanceof WikiError;
 64+ if ( $object instanceof WikiError ) {
 65+ return true;
 66+ } elseif ( $object instanceof Status ) {
 67+ return !$object->isOK();
 68+ } else {
 69+ return false;
 70+ }
6571 }
6672 }
6773
Index: trunk/phase3/includes/Preferences.php
——@@ -1272,8 +1272,8 @@
12731273 # Mail a temporary password to the dirty address.
12741274 # User can come back through the confirmation URL to re-enable email.
12751275 $result = $wgUser->sendConfirmationMail( $oldaddr != '' );
1276 - if ( WikiError::isError( $result ) ) {
1277 - return wfMsg( 'mailerror', htmlspecialchars( $result->getMessage() ) );
 1276+ if ( !$result->isGood() ) {
 1277+ return htmlspecialchars( $result->getWikiText( 'mailerror' ) );
12781278 } elseif ( $entryPoint == 'ui' ) {
12791279 $result = 'eauth';
12801280 }
Index: trunk/phase3/languages/messages/MessagesEn.php
——@@ -1144,6 +1144,11 @@
11451145 * Nederlands|nl', # do not translate or duplicate this message to other languages
11461146 'suspicious-userlogout' => 'Your request to log out was denied because it looks like it was sent by a broken browser or caching proxy.',
11471147
 1148+# E-mail sending
 1149+'pear-mail-error' => '$1', # do not translate or duplicate this message to other languages
 1150+'php-mail-error' => '$1', # do not translate or duplicate this message to other languages
 1151+'php-mail-error-unknown' => "Unkown error in PHP's mail() function",
 1152+
11481153 # JavaScript password checks
11491154 'password-strength' => 'Estimated password strength: $1',
11501155 'password-strength-bad' => 'BAD',

Follow-up revisions

RevisionCommit summaryAuthorDate
r77797Fix typo in r77714catrope12:58, 5 December 2010
r83270(bug 27862; follow-up r77714) Make emailuser api module not freak out when Sp...bawolff03:35, 5 March 2011

Comments

#Comment by Catrope (talk | contribs)   12:51, 5 December 2010 
+		if( !$status->isGood() && false ) {

I know this wasn't introduced in this rev, but this is dead code. Will track it down.

#Comment by Catrope (talk | contribs)   12:54, 5 December 2010

Introduced in r64903, commented there.

Status & tagging log