r47529 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r47528‎ | r47529 | r47530 >
Date:03:47, 20 February 2009
Author:tstarling
Status:deferred (Comments)
Tags:
Comment:
* Removed MW 1.9 compatibility
* Use early returns to avoid excessive indenting
* Fixed a bug causing the "email me a copy" feature to not work if $wgUserEmailUseReplyTo=false
* Make the "email me a copy" emails come from the contact sender instead of the person who submitted the form. Makes more sense to me.
* Rename $from and $to so we don't have to send mails to from, or from to
Modified paths:
  • /trunk/extensions/ContactPage/SpecialContact.php (modified) (history)

Diff [purge]

Index: trunk/extensions/ContactPage/SpecialContact.php
——@@ -15,9 +15,6 @@
1616 die( 1 );
1717 }
1818
19 -global $IP; #needed when called from the autoloader
20 -require_once("$IP/includes/UserMailer.php");
21 -
2219 /**
2320 * Provides the contact form
2421 * @ingroup SpecialPage
——@@ -271,16 +268,17 @@
272269
273270 wfDebug( __METHOD__ . ": start\n" );
274271
275 - $to = new MailAddress( $this->target );
276 - $replyto = NULL;
 272+ $targetAddress = new MailAddress( $this->target );
 273+ $replyto = null;
 274+ $contactSender = new MailAddress( $csender, $cname );
277275
278276 if ( !$this->fromaddress ) {
279 - $from = new MailAddress( $csender, $cname );
280 - } else if ( $wgUserEmailUseReplyTo ) {
281 - $from = new MailAddress( $csender, $cname );
282 - $replyto = new MailAddress( $this->fromaddress, $this->fromname );
 277+ $submitterAddress = $contactSender;
283278 } else {
284 - $from = new MailAddress( $this->fromaddress, $this->fromname );
 279+ $submitterAddress = new MailAddress( $this->fromaddress, $this->fromname );
 280+ if ( $wgUserEmailUseReplyTo ) {
 281+ $replyto = $submitterAddress;
 282+ }
285283 }
286284
287285 $subject = trim( $this->subject );
——@@ -295,48 +293,49 @@
296294 $subject = wfMsgForContent( 'contactpage-subject-and-sender', $subject, $this->fromaddress );
297295 }
298296
299 - if( wfRunHooks( 'ContactForm', array( &$to, &$replyto, &$subject, &$this->text ) ) ) {
 297+ if( !wfRunHooks( 'ContactForm', array( &$targetAddress, &$replyto, &$subject, &$this->text ) ) ) {
 298+ wfDebug( __METHOD__ . ": aborted by hook\n" );
 299+ return;
 300+ }
300301
301 - wfDebug( __METHOD__ . ": sending mail from ".$from->toString()." to ".$to->toString()." replyto ".( $replyto == null ? '-/-' : $replyto->toString() )."\n" );
 302+ wfDebug( __METHOD__ . ": sending mail from ".$submitterAddress->toString().
 303+ " to ".$targetAddress->toString().
 304+ " replyto ".( $replyto == null ? '-/-' : $replyto->toString() )."\n" );
302305
303 - #HACK: in MW 1.9, replyto must be a string, in MW 1.10 it must be an object!
304 - $ver = preg_replace( '![^\d._+]!', '', $GLOBALS['wgVersion'] );
305 - $replyaddr = $replyto == null
306 - ? NULL : version_compare( $ver, '1.10', '<' )
307 - ? $replyto->toString() : $replyto;
 306+ $mailResult = UserMailer::send( $targetAddress, $submitterAddress, $subject, $this->text, $replyto );
308307
309 - $mailResult = userMailer( $to, $from, $subject, $this->text, $replyaddr );
 308+ if( WikiError::isError( $mailResult ) ) {
 309+ $wgOut->addWikiMsg( 'usermailererror' ) . $mailResult->getMessage();
 310+ wfDebug( __METHOD__ . ": got error from UserMailer: " . $mailResult->getMessage() . "\n" );
 311+ return;
 312+ }
310313
311 - if( WikiError::isError( $mailResult ) ) {
312 - $wgOut->addWikiMsg( 'usermailererror' ) . $mailResult->getMessage();
313 - } else {
314 - // if the user requested a copy of this mail, do this now,
315 - // unless they are emailing themselves, in which case one copy of the message is sufficient.
316 - if( $this->cc_me && $replyto ) {
317 - $cc_subject = wfMsg('emailccsubject', $this->target->getName(), $subject);
318 - if( wfRunHooks( 'ContactForm', array( &$from, &$replyto, &$cc_subject, &$this->text ) ) ) {
319 - wfDebug( __METHOD__ . ": sending cc mail from ".$from->toString()." to ".$replyto->toString()."\n" );
320 - $ccResult = userMailer( $replyto, $from, $cc_subject, $this->text );
321 - if( WikiError::isError( $ccResult ) ) {
322 - // At this stage, the user's CC mail has failed, but their
323 - // original mail has succeeded. It's unlikely, but still, what to do?
324 - // We can either show them an error, or we can say everything was fine,
325 - // or we can say we sort of failed AND sort of succeeded. Of these options,
326 - // simply saying there was an error is probably best.
327 - $wgOut->addWikiText( wfMsg( 'usermailererror' ) . $ccResult );
328 - return;
329 - }
330 - }
 314+ // if the user requested a copy of this mail, do this now,
 315+ // unless they are emailing themselves, in which case one copy of the message is sufficient.
 316+ if( $this->cc_me && $this->fromaddress ) {
 317+ $cc_subject = wfMsg('emailccsubject', $this->target->getName(), $subject);
 318+ if( wfRunHooks( 'ContactForm', array( &$submitterAddress, &$contactSender, &$cc_subject, &$this->text ) ) ) {
 319+ wfDebug( __METHOD__ . ": sending cc mail from ".$contactSender->toString().
 320+ " to ".$submitterAddress->toString()."\n" );
 321+ $ccResult = UserMailer::send( $submitterAddress, $contactSender, $cc_subject, $this->text );
 322+ if( WikiError::isError( $ccResult ) ) {
 323+ // At this stage, the user's CC mail has failed, but their
 324+ // original mail has succeeded. It's unlikely, but still, what to do?
 325+ // We can either show them an error, or we can say everything was fine,
 326+ // or we can say we sort of failed AND sort of succeeded. Of these options,
 327+ // simply saying there was an error is probably best.
 328+ $wgOut->addWikiText( wfMsg( 'usermailererror' ) . $ccResult );
 329+ return;
331330 }
332 -
333 - wfDebug( __METHOD__ . ": success\n" );
334 -
335 - $titleObj = SpecialPage::getTitleFor( 'Contact' );
336 - $wgOut->redirect( $titleObj->getFullURL( "action=success" ) );
337 - wfRunHooks( 'ContactFromComplete', array( $to, $replyto, $subject, $this->text ) );
338331 }
339332 }
340333
 334+ wfDebug( __METHOD__ . ": success\n" );
 335+
 336+ $titleObj = SpecialPage::getTitleFor( 'Contact' );
 337+ $wgOut->redirect( $titleObj->getFullURL( "action=success" ) );
 338+ wfRunHooks( 'ContactFromComplete', array( $targetAddress, $replyto, $subject, $this->text ) );
 339+
341340 wfDebug( __METHOD__ . ": end\n" );
342341 }
343342
——@@ -348,4 +347,4 @@
349348
350349 $wgOut->returnToMain( false );
351350 }
352 -}
\ No newline at end of file
 351+}

Comments

#Comment by GreenReaper (talk | contribs)   21:13, 31 July 2009

The section in doSubmit where it works out the correct From and ReplyTo addresses is incorrect and breaks the intended functionality of $wgUserEmailUseReplyTo (sending from the server address, so it gets past spam filters that check that the origin has the authority to send email as that person).

I suggest the following code instead: 

               if ( !$this->fromaddress ) {
                       $submitterAddress = $contactSender;
               } else {
                       if ( $wgUserEmailUseReplyTo ) {
                               $submitterAddress = $contactSender;
                               $replyto = $submitterAddress;
                       } else {
                               $submitterAddress = new MailAddress( $this->fromaddress, $this->fromname );
                       }
               }
#Comment by GreenReaper (talk | contribs)   21:30, 31 July 2009

Or shorter and more correctly . . . 

              if ( !$this->fromaddress ) {
                      $submitterAddress = $contactSender;
              } else {
                      $submitterAddress = new MailAddress( $this->fromaddress, $this->fromname );
                      if ( $wgUserEmailUseReplyTo ) {
                              # Use the submitter as Reply-To: and the standard contact address as From:
                              # Avoids spam filters that notice we're sending from a domain we don't run
                              $replyto = $submitterAddress;
                              $submitterAddress = $contactSender;
                      }
              }

Status & tagging log