Page MenuHomePhabricator

Correct parameter docs in UserMailer::sendWithPear()
Closed, ResolvedPublic

Description

protected static function sendWithPear( $mailer, $dest, $headers, $body ) {
		$mailResult = $mailer->send( $dest, $headers, $body );


	public static function send( $to, $from, $subject, $body, $replyto = null,
		$contentType = 'text/plain; charset=UTF-8'
	) {

$from parameter is missing. $headers being the same as $subject seems wrong


Version: 1.24rc
Severity: normal

Details

Reference
bz66673
Related Gerrit Patches:

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:14 AM
bzimport added a project: MediaWiki-Email.
bzimport set Reference to bz66673.
bzimport added a subscriber: Unknown Object (MLST).
Reedy created this task.Jun 16 2014, 5:09 PM

Is this an "easy" bug (keyword)? If anybody wants to work on this, the file is /includes/UserMailer.php in MediaWiki core.

demon set Security to None.
demon lowered the priority of this task from Medium to Low.Jun 11 2015, 4:31 PM
demon raised the priority of this task from Low to High.
demon added subscribers: demon, Legoktm.
demon lowered the priority of this task from High to Low.Jun 11 2015, 4:37 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 25 2016, 1:31 PM

Change 273228 had a related patch set uploaded (by Rosalieper):
Correcting parameters for UserMailer::sendWithPear() which calls $mailer->send()

https://gerrit.wikimedia.org/r/273228

I am just wondering, if we are doing the right thing here. The send() in this command is not UserMailer::send(), but
Mail_smtp::send() of Pear mailer https://pear.php.net/package/Mail/docs/latest/Mail/Mail.html#methodsend

The $mail_object is actually a Pear instance

$mail_object =& Mail::factory( 'smtp', $wgSMTP );

and it takes in the argument as

$recipients , $headers, $body

which seems to be correct as per the definition in https://github.com/wikimedia/mediawiki/blob/master/includes/mail/UserMailer.php#L44

Reedy added a comment.Feb 25 2016, 2:19 PM

I am just wondering, if we are doing the right thing here. The send() in this command is not UserMailer::send(), but
Mail_smtp::send() of Pear mailer https://pear.php.net/package/Mail/docs/latest/Mail/Mail.html#methodsend
The $mail_object is actually a Pear instance

$mail_object =& Mail::factory( 'smtp', $wgSMTP );

and it takes in the argument as

$recipients , $headers, $body

which seems to be correct as per the definition in https://github.com/wikimedia/mediawiki/blob/master/includes/mail/UserMailer.php#L44

So the docs are wrong?

Should be Mail_smtp then...

Reedy updated the task description. (Show Details)Feb 25 2016, 2:21 PM
Reedy added a subscriber: rosalieper.
rosalieper added a comment.EditedFeb 25 2016, 2:25 PM

But @01tonythomas, the send() method has been defined there meaning if $mailer is calling that method, then its using the definition of the send() defined. I don't know whether i am making some sense though or is the send() defined in the UserMailer.php file suppose to somehow override the PEAR send() method?

Reedy added a comment.Feb 25 2016, 2:29 PM
			// Create the mail object using the Mail::factory method
			$mail_object =& Mail::factory( 'smtp', $wgSMTP );
			if ( PEAR::isError( $mail_object ) ) {
				wfDebug( "PEAR::Mail factory failed: " . $mail_object->getMessage() . "\n" );
				MediaWiki\restoreWarnings();
				return Status::newFatal( 'pear-mail-error', $mail_object->getMessage() );
			}

			wfDebug( "Sending mail via PEAR::Mail\n" );

			$headers['Subject'] = self::quotedPrintable( $subject );

			// When sending only to one recipient, shows it its email using To:
			if ( count( $to ) == 1 ) {
				$headers['To'] = $to[0]->toString();
			}

			// Split jobs since SMTP servers tends to limit the maximum
			// number of possible recipients.
			$chunks = array_chunk( $to, $wgEnotifMaxRecips );
			foreach ( $chunks as $chunk ) {
				$status = self::sendWithPear( $mail_object, $chunk, $headers, $body );
	/**
	 * Send mail using a PEAR mailer
	 *
	 * @param UserMailer $mailer
	 * @param string $dest
	 * @param string $headers
	 * @param string $body
	 *
	 * @return Status
	 */
	protected static function sendWithPear( $mailer, $dest, $headers, $body ) {
		$mailResult = $mailer->send( $dest, $headers, $body );

		// Based on the result return an error string,
		if ( PEAR::isError( $mailResult ) ) {
			wfDebug( "PEAR::Mail failed: " . $mailResult->getMessage() . "\n" );
			return Status::newFatal( 'pear-mail-error', $mailResult->getMessage() );
		} else {
			return Status::newGood();
		}
	}

I think

	 * @param UserMailer $mailer

wants to be

	 * @param Mail_smtp $mailer

as per what Tony is saying...

We're passing 'smtp' to factory. The factory code is

/**
 * Provides an interface for generating Mail:: objects of various
 * types
 *
 * @param string $driver The kind of Mail:: object to instantiate.
 * @param array  $params The parameters to pass to the Mail:: object.
 * @return object Mail a instance of the driver class or if fails a PEAR Error
 * @access public
 */
function &factory($driver, $params = array())
{
    $driver = strtolower($driver);
    @include_once 'Mail/' . $driver . '.php';
    $class = 'Mail_' . $driver;
    if (class_exists($class)) {
        $mailer = new $class($params);
        return $mailer;
    } else {
        return PEAR::raiseError('Unable to find class for driver ' . $driver);
    }
}

So it's going to return a Mail_$FOOBAR class, and in our case, it's Mail_smtp

01tonythomas added a comment.EditedFeb 25 2016, 2:32 PM

yeah. I guess, what needs to be changed would be just the docs, as we are not calling UserMailer::send(), but Pears on Mail_smtp:send().

as per

array $headers - an associative array of headers. The header name is used as key and the header value as value. If you want to override the envelope sender of the email, set the Return-Path header and that value will be used instead of the value of the From: header.

It fetch the $from correctly from $headers too :) Thank you @Reedy for pasting the refrence!

@Reedy, thats correct. Its makes sense, meaning the previous code was ok. So the send() method called in sendWithPear() in this case is that of Mail_smtp which is true. Thanks @01tonythomas, meaning we just have to update the documention for the code so we specify that $mailer is using Mail_smtp send() method. Is that right @Reedy, @01tonythomas ?

demon removed a subscriber: demon.Feb 25 2016, 3:25 PM

Change 273228 merged by jenkins-bot:
Updating docs for UserMailer::sendWithPear() which calls $mailer->send()

https://gerrit.wikimedia.org/r/273228

01tonythomas renamed this task from UserMailer::sendWithPear() calls $mailer->send() with incorrect parameters to Correct parameter docs in UserMailer::sendWithPear().Mar 8 2016, 4:49 PM

This one is resolved, fixed after https://gerrit.wikimedia.org/r/#/c/273228/. Time to mark this done ?

demon closed this task as Resolved.Feb 8 2017, 10:59 PM
mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:12 PM