SimpleTest should override all mail system implementations with TestMailCollector

Yes, I think this is a very good idea.

The mail.system variable was dysfunctional in D7 - read my explanation at https://drupal.org/comment/4160574#comment-4160574. So one could argue that the D7 behavior was a bug and that D8 uses the improved config system to correct that. Which is true, but only for live sites. SimpleTest is another matter entirely.

The whole point of resetting this variable in SimpleTest is so that SimpleTest can capture any outgoing mail in order to test the mail functionality. Withoug this change, we cannot ever test a module that implements its own mail system.

So a big +1 for this change - it doesn't impact any tests unless you have your own mail system defined, and if you do it allows you to test your module, which is currently impossible without workarounds.

Nice catch. This looks like a typo; the system.mail.yml file defines this as:

interface:
 default: 'Drupal\Core\Mail\PhpMail'

... so I don't really see how the old code would've ever worked.

My only question is whether it's possible to add a test for this?

Following, let's do this after #2187495: Use plugin system for MailInterface classes. There is a test module but I can't see a test that would verify it? Also, the comment in the test module still references the old class name, but all of that should switch to plugins anyway after the pluginification.

This will also need a reroll because of #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead, which substantially rewrote WebTestBase.

There are tests that manipulate system.mail settings beyond what WebTestBase does; it would probably be a good idea to change those, as well. Here's what I found on a grep:

\Drupal\user\Tests\UserCreateFailMailTest:

    \Drupal::config('system.mail')->set('interface.default', 'Drupal\system_mail_failure_test\TestPhpMailFailure')->save();

\Drupal\system\Tests\InstallerTest:

    \Drupal::config('system.mail')
      ->set('interface.default', 'Drupal\Core\Mail\TestMailCollector')
      ->save();

\Drupal\system_mail_failure_test\TestPhpMailFailure:

/**
 * @code
 *   \Drupal::config('system.mail')->set('interface.default', 'Drupal\system_mail_failure_test\TestPhpMailFailure')->save();
 * @endcode
 */

\Drupal\system\Tests\Mail\MailTest does this as well, but the body of that test will change a lot if #2187495: Use plugin system for MailInterface classes lands.

rerolled

rerolled

rerolled

rerolled

Updated Rerole.

ReRolled.

It looks like something has been lost in the last 2 rerolls compared to the patches in #14 and #19.

Re-rolled patch. Added back missing elements.

Fixed the PHP error. Uploading the patch again.

Reroll attached.

Removing empty .module file per (.module and .profile files are no longer required; ModuleHandler::getModuleList() now returns Extension objects).

It's failing because the patch doesn't take into account the changes from #2392319: Config objects (but not config entities) should by default be immutable

Adding the module file back in, to see if that is needed here. See what simpletest says.

Correcting because of #40, and re-removing the empty .module file (see #37).

Is the following desirable still from #14, 19, 26, 28 and 29 but not in 33 or 35?

diff --git a/core/modules/simpletest/src/InstallerTestBase.php b/core/modules/simpletest/src/InstallerTestBase.php
index 2065075..bb6d154 100644
--- a/core/modules/simpletest/src/InstallerTestBase.php
+++ b/core/modules/simpletest/src/InstallerTestBase.php
@@ -161,7 +161,7 @@ protected function setUp() {
     // Manually configure the test mail collector implementation to prevent
     // tests from sending out e-mails and collect them in state instead.
     $config->get('system.mail')
-      ->set('interface.default', 'test_mail_collector')
+      ->set('interface', array('default' => 'test_mail_collector'))
       ->save();
 
     $this->isInstalled = TRUE;

1. +++ b/core/modules/simpletest/src/Tests/MailCaptureTest.php
   @@ -17,6 +17,13 @@
    class MailCaptureTest extends WebTestBase {
      /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  public static $modules = array('mailsystem_test');
   +  ¶
   +  /**
   

   Nitpicks:

   There should be an empty line after the class and the docblock.

   Trailing spaces after the $modules lines.

2. +++ b/core/modules/simpletest/tests/modules/mailsystem_test/mailsystem_test.info.yml
   @@ -0,0 +1,7 @@
   +core: 8.x
   +hidden: TRUE
   \ No newline at end of file
   
   +++ b/core/modules/simpletest/tests/modules/mailsystem_test/mailsystem_test.install
   @@ -0,0 +1,12 @@
   +}
   \ No newline at end of file
   

   make sure there's a newline at the end of those files, git doesn't like it if isn't.

Maybe a duplicate of #2609680: Add an AssertMailTrait to allow mail testing in Kernel tests and fix odd stdout not found in test output

1. +++ b/core/modules/simpletest/src/Tests/MailCaptureTest.php
   @@ -16,6 +16,14 @@
   +  ¶
   

   trailing whitespace

2. +++ b/core/modules/simpletest/tests/modules/mailsystem_test/mailsystem_test.info.yml
   @@ -0,0 +1,7 @@
   \ No newline at end of file
   

   (

Maybe a duplicate of #2609680

Agreed. (Although I think they had the wrong solution, as I stated there.)