Allow PHPMailer class to be reliably overriden

[leewillis77]

The WordPress unit test suite currently overrides the global $phpmailer variable with its own MockMailer class. (See https://core.trac.wordpress.org/browser/trunk/tests/phpunit/includes/bootstrap.php#L48)

This achieves the desired effect, but is potentially unreliable.

If the global $phpmailer object gets unset, or overridden (For example by a buggy, or otherwise badly behaving plugin), then the code in wp-includes/pluggable.php will create a new instance of PHPMailer without any way for code to override that - see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/pluggable.php#L264

The end result would be that a site that shouldn't be sending emails will potentially become one that will.

Potential patch attached. This includes the change to pluggable.php to filter the object after creation, and addition to the test suite to use it.

[nacin]

I don't hate this, so that's good. :) Will want some other feedback.

Another option: Note that this object does basically nothing on construct so we could probably just always construct it and work around it in our tests some other way.

[chriscct7]

Patch still applies but per comment:1 needs adjustment

[desrosj]

28618.diff​ needs to be refreshed, and the comments above still need to be addressed.

[leewillis77]

I'm not clear that the patch does need to address the comments in #1. They are a potential improvement over-and-above this change, not critical to the actual issue (unless I've misunderstood)?

[SirLouen]

Replying to leewillis77:

For example by a buggy, or otherwise badly behaving plugin

@leewillis77 can you provide ​a demo use case for this? More or less I can see your proposal, but I can see a real practical use without messing too much with the tests code.

I can't really see a case where a plugin can be actually interfering with the test suite (unless it's actually your plugin and you are testing for it).

Also, I don't really get what @nacin is proposing. In conclusion, this needs-test-info to proceed.

I provide a refreshed patch, just in case.

This is a refresh for 28618.diff
Although more Test Info is needed to proceed with further testing and analysis.

Trac ticket: https://core.trac.wordpress.org/ticket/28618

[SirLouen]

After re-reading and trying to play around with this, I still don't really understand how this can happen from outside the PHPUnit suite.

But if we focus on a Unit Tests perspective, it's true that unsetting the global, brings a real PHP Mailer object instead of keeping it Mock. I've added a Unit Test to demo this.

The idea behind this report is definitely legit, but personally, I don't love this kind of reports that I cannot prove them with a real life case.

Despite personally doing some work on this report and demonstrating the issue with a unit test and refreshing the patch, I'm going to set this to close as for me, it's totally unclear what value is providing. I would rather prefer to close this ticket if no answer is given and maybe if someone happens to find it in the future or the reporter happens to provide some extra info, consider reopening it and further working on reviewing all this with more data at hand.

[SirLouen]

Ok, after doing a little more research, yes I could reproduce this in a single test, but I could not reproduce in a plugin as the reporter suggested, because I forgot that tests run in a different DB, and there should not be any active_plugins in the wp_options table by default unless we are doing something very wrong. This plugin thing was misdirecting my attention.

But now I've been looking into how the tear_down in wpMail is running the reset_phpmailer_instance.

The reality is that, if we test this way:

/**
 * @group sample
 */
public function test_unsetting_phpmailer_instance() {
	global $phpmailer;
	$phpmailer = null;
	$this->assertNull( $phpmailer );
}
/**
 * @group sample
 */
public function test_random_delivery() {
	wp_mail( 'test@example.com', 'Test', 'Test' );
	global $phpmailer;
	$this->assertInstanceOf( 'MockPHPMailer', $phpmailer );
}

The PHPMailer instance is not being completely reset in tear_down in reset_phpmailer_instance and the email is being sent with a regular PHPMailer object.

This section of code is not doing the job:

​https://github.com/WordPress/wordpress-develop/blob/3ea6be9fa8a4973c2b01c094f062de4fd33c0aba/tests/phpunit/includes/mock-mailer.php#L110-L123

The problem is that the $mailer conditional is not covering the scenario where $phpmailer is unset.

This was addressed in [41185], but was not sufficiently addressed, not covering this edge case. What I'm trying to devise is why the conditional was introduced first in [37358].

I think that the object should be reset unconditionally. I would like to see a patch proposal.

This PR is not valid anymore.

added filter to make it simpler to override the global mailer change tests to use new filter

Trac ticket: https://core.trac.wordpress.org/ticket/28618