Use plugin system for MailInterface classes

Agreed that this would be great to have but it would be good to have this approved as an API change before you waste time on it if it won't be possible to get it in. Tagging as API change and beta target, as it probably has to happen before beta.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Mail/PhpMail.php
@@ -2,13 +2,20 @@
  * @file
- * Definition of Drupal\Core\Mail\PhpMail.
+ * Definition of Drupal\system\Plugin\Mail\PhpMail.
  */
 

components can provide plugins too, it just work just fine if you have it in Drupal\Core\Mail\Plugin\Mail\PhpMail.

We started experimenting with a plugin manager and a more flexible mail factory for mailsystem as well, as we want to improve the ability to use separate plugins for send and format there. We just added the core implementation as a hardcoded list. Wondering what's the best way to combine them.

Previously, mailsystem used to find implementations by searching the registry table for a given class suffix, similar to how simpletest works in 7.x, that is now impossible, so we need some kind of discovery.

This change looks like a good idea. In fact because of

Previously, mailsystem used to find implementations by searching the registry table for a given class suffix, similar to how simpletest works in 7.x, that is now impossible, so we need some kind of discovery.

you could argue that this is a regression. If @Berdir and @Les Lim can agree a scope and a way forward that is as limited as possible and with the least impact of other parts of Drupal I'm +1 to doing this.

1. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/TestMailCollector.php
   @@ -17,13 +17,15 @@
   + *   label = @Translation("Mail Collector"),
   + *   description = @Translation("Does not send the message, but stores it in Drupal within the state system. Used for development and testing.")
   

   I think you can leave out the "for development", this is really only about testing.

2. +++ b/core/modules/system/tests/modules/system_mail_failure_test/lib/Drupal/system_mail_failure_test/Plugin/Mail/TestPhpMailFailure.php
   @@ -21,13 +21,15 @@
    
      /**
   -   * Overrides Drupal\system\Plugin\Mail\PhpMail::mail().
   +   * Overrides Drupal\Core\Mail\Plugin\Mail\PhpMail::mail().
       */
   

   When changing Overrides/Implements docblock, just change them to {@inheritdoc}

Looks great.

I think we will want to update/expand the existing change record at https://drupal.org/node/2073215. You could already prepare an updated version of that in the issue summary here, the new change record draft system can't handle new revisions yet I think :)

1. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -0,0 +1,137 @@
   +  public function get($module, $key) {
   

   I think this should use getInstance, which is on MapperInterface (which all plugin managers implement).

   See FormatterPluginManager, WidgetPluginManager, SelectionPluginManager, or ResourcePluginManager for example implementations

2. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -0,0 +1,137 @@
   +        throw new \Exception(String::format('Class %class does not implement interface %interface', array('%class' => get_class($plugin), '%interface' => 'Drupal\Core\Mail\MailInterface')));
   

   I can't remember offhand, but we usually use a more specific exception class for this

3. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -0,0 +1,137 @@
   +      $interfaces = class_implements($plugin);
   +      if (isset($interfaces['Drupal\Core\Mail\MailInterface'])) {
   

   Use is_subclass_of

4. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -0,0 +1,137 @@
   +    $this->mailConfig = $configFactory->get('system.mail');
   ...
   +  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ModuleHandlerInterface $module_handler, ConfigFactory $configFactory) {
   

   $config_factory

5. +++ b/core/modules/system/tests/modules/system_mail_failure_test/lib/Drupal/system_mail_failure_test/Plugin/Mail/TestPhpMailFailure.php
   @@ -0,0 +1,38 @@
   + *   id = "TestPhpMailFailure",
   

   There's no written rule for this, but every other plugin ID in core is lower_snake_case, not UpperCamelCase.

1. +++ b/core/includes/mail.inc
   @@ -189,7 +189,7 @@ function drupal_mail($module, $key, $to, $langcode, $params = array(), $reply =
    
    /**
   - * Returns an object that implements Drupal\Core\Mail\MailInterface.
   + * Returns an instance of the mail plugin to use for a given message ID.
     *
   
   @@ -198,14 +198,14 @@ function drupal_mail($module, $key, $to, $langcode, $params = array(), $reply =
     *
     * @return \Drupal\Core\Mail\MailInterface
   - *   An object that implements Drupal\Core\Mail\MailInterface.
   + *   A mail plugin object.
     *
   

   First you change object to instance but below you keep it? Should probably use instance in both cases?

2. +++ b/core/modules/system/tests/modules/system_mail_failure_test/lib/Drupal/system_mail_failure_test/Plugin/Mail/TestPhpMailFailure.php
   @@ -0,0 +1,38 @@
   +
   +  /**
   +   * Overrides Drupal\Core\Mail\Plugin\Mail\PhpMail::mail().
   +   *
   +   * Intentionally fails to send the message.
   +   */
   +  public function mail(array $message) {
   +    return FALSE;
   +  }
   

   Our coding standards have nothing to say about this, so I'm not sure. There are issues to discuss this, so let's not waste time on this :)

   Looks like git doesn't see this file as a move anymore, maybe you can use lower -M numbers (e.g. git diff -M25) to force it, would make the changes easier to review. Don't worry if it doesn't work, it's a small class anyway.

Looks good otherwise I think, change notice also looks great.

+++ b/core/modules/system/lib/Drupal/system/Tests/Mail/MailTest.php
@@ -8,20 +8,19 @@
 /**
  * Defines a mail class used for testing.
  */
-class MailTest extends WebTestBase implements MailInterface {
+class MailTest extends WebTestBase {
 

The idea to use the same class not only for the test but also an actual test implementation is so crazy, yay for cleaning that up :)

This looks awesome, and now the diff is very easy to understand and shows what is changing, great. This is RTBC I think :)

Also tagging as approved API change, based on alexpott's comment above.

core-2187495-26-mail-to-plugins.patch no longer applies.

error: patch failed: core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php:846
error: core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php: patch does not apply
error: patch failed: core/modules/system/lib/Drupal/system/Tests/InstallerTest.php:75
error: core/modules/system/lib/Drupal/system/Tests/InstallerTest.php: patch does not apply

29: core-2187495-29-mail-to-plugins.patch queued for re-testing.

Lets create a change record before commit https://groups.drupal.org/node/402688

While someone's looking at the mail system could they review #2146971: SimpleTest should override all mail system implementations with TestMailCollector? It will need rerolling after this gets in, should be simple enough though.

+++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
@@ -2,13 +2,21 @@
+ * @Plugin(

+++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/TestMailCollector.php
@@ -2,21 +2,29 @@
+ * @Plugin(

+++ b/core/modules/system/tests/modules/system_mail_failure_test/lib/Drupal/system_mail_failure_test/Plugin/Mail/TestPhpMailFailure.php
@@ -16,16 +16,23 @@
+ * @Plugin(

Can we implement an @Mail annotation. See https://api.drupal.org/api/drupal/core%21modules%21block%21lib%21Drupal%...

I'm not

+++ b/core/lib/Drupal/Core/Mail/MailManager.php
@@ -23,104 +28,115 @@ class MailFactory {
+    parent::__construct('Plugin/Mail', $namespaces);

I'm not sure how this passes, all of the other managers using custom annotations have to specify it here.

Also, why are we not providing an alter hook? The rest of the managers do.

Considering https://drupal.org/node/2187495#comment-8471327 did not fail we must be missing test coverage. No? Or put anyway way how did it pass?

From an IRC conversation with @Les Lim

16:54 Les_Lim alexpott: well, to recap, we didn't specify an annotation class to use for plugin discovery, so DefaultPluginManager assumed 'Drupal\Component\Annotation\Plugin'
16:55 alexpott Les_Lim: but then how did it discover the @Mail annotations?
16:55 Les_Lim alexpott: all annotation classes inherit from 'Drupal\Component\Annotation\Plugin'
16:55 Les_Lim i think it just does an instanceof to check
16:55 Les_Lim alexpott: but I'm not sure that's actually a bug
16:56 alexpott Les_Lim: me neither - it is interesting :)
16:57 Les_Lim alexpott: it got all the proper plugins, because they were in the right subdir

The mail plugins define a label and description, but nothing uses them, and there is no test coverage for them - maybe that would have picked up the incorrect annotation config?

I don't think so, you can put anything on an annotation, the classes are just for documentation.

That said, it would probably be useful to have some explicit test coverage of the plugin manager, and get the definitions and verify the label in there?

Overriding getInstance() is not very common, and at least having a unit test for that would be useful.

What I was suggesting wouldn't be a unit test as we currently can not unit test annotation based plugin discovery due to @Translation which calls t(). But yes, unit tests for the getInstance() would possibly be useful.

Please note that we have two plugin concepts baked into our current Mail API right now:

1. Mail formatting

   The gist of format(). Typically implemented by Mimemail module and others, which only care about formatting (and headers), but do NOT care for how the mail is sent in the end.

2. Mail sending

   The gist of send(). Typically implemented by SMTP module and PHPMailer module, which only (or mostly) care about mail sending, but do not care for how the mail is formatted (as long as mail headers are reflecting the chosen encoding).

I originally assumed that a proper separation of the two concepts was still an unresolved issue for D8, but after grepping core issues, I was only able to find #331180: fix pluggable smtp/mail framework, which is fixed since D7 already.

In any case, a plugin-ification of the mail system should take this separation of concerns into account. — That is, because there is hardly a use-case for a mail plugin in the wild that implements both. - Above mentioned (prime) examples are just a testament to that.

Unless I missed something, the clean separation should exist in core already. It is merely possible that the default PhpMail implementation happens to be a provider for both plugin types, because Drupal core does not support any other format than plain-text. However, that coincidence does not and should not mean that a separation of concerns was not foreseen. :-)

I would really prefer to not complicate this issue further by splitting that up, as much as I agree it would be nicer.

This is already so much better than what we had in 7.x, we already have an initial 8.x port of mailsystem that extends the MailManager and uses an adapter class to make it possible to select two different classes for format() and send(). No longer required to generate code for that as in 7.x :)

No reason to make split here. Better to file follow-up for formatting questions, including pointed in #1856562-11: Convert "Subject" and "Message" into Message base fields (2,3)

1. +++ b/core/lib/Drupal/Core/Annotation/Mail.php
   @@ -0,0 +1,44 @@
   +  public $description;
   
   +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
   @@ -2,13 +2,21 @@
   + *   description = @Translation("Sends the message as plain text, using PHP's native mail() function.")
   
   +++ b/core/modules/system/tests/modules/system_mail_failure_test/lib/Drupal/system_mail_failure_test/Plugin/Mail/TestPhpMailFailure.php
   @@ -16,16 +16,23 @@
   + *   description = @Translation("An intentionally broken mail backend, used for tests.")
   

   I think description is useless, This should be placed in class description.
   Also "name" could be more informative

2. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php
   @@ -2,13 +2,21 @@
   - * Definition of Drupal\Core\Mail\PhpMail.
   + * Definition of Drupal\Core\Mail\Plugin\Mail\PhpMail.
   

   Contains

3. +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/TestMailCollector.php
   @@ -2,21 +2,29 @@
   -   * Overrides \Drupal\Core\Mail\PhpMail::mail().
   +   * Overrides \Drupal\Core\Mail\Plugin\Mail\PhpMail::mail().
   
   +++ b/core/modules/system/tests/modules/system_mail_failure_test/lib/Drupal/system_mail_failure_test/Plugin/Mail/TestPhpMailFailure.php
   @@ -16,16 +16,23 @@
   -   * Overrides Drupal\Core\Mail\PhpMail::mail().
   +   * Overrides Drupal\Core\Mail\Plugin\Mail\PhpMail::mail().
   

   @inheritdoc

1. Description isn't useless when the plugins are exposed in the UI, e.g. in mailsystem.

3. This adds additional comments. Per our current coding standards, it is not allowed to use @inheritdoc there. That said, it would IMHO be perfectly fine to keep the inline comment there instead. I would say that we don't override the parent class but implement the method defined in the interface and @inheritdoc points to that. Then the inline comment explains that our implementation is simply "always fail".

This patch improves some of the comments and labelling. I agree that "description" may be useful in contrib.

Wrong interdiff.

@longwave: Those two fails are identical?

Pro-tip: Do not name your interdiff's interdiff.txt. As you can see, they get renamed to interdiff_xxxx.txt, and the way drupal.org figures out what number to use, it has to loop from 1 to (currently) 2386, checking if the file already exists on every loop. That's not nice.

1. +++ b/core/tests/Drupal/Tests/Core/Mail/MailManagerTest.php
   @@ -0,0 +1,163 @@
   +
   +    // Mock a CachedDiscovery object to replace AnnotationClassDiscovery.
   +    $this->discovery = $this->getMock('Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface');
   +    foreach ($this->definitions as $id => $definition) {
   +      $definitions_map[] = array($id, $definition);
   +    }
   +    $this->discovery->expects($this->any())
   +      ->method('getDefinition')
   +      ->will($this->returnValueMap($definitions_map));
   +    $this->discovery->expects($this->any())
   +      ->method('getDefinitions')
   +      ->will($this->returnValue($this->definitions));
   

   I assume you copied that from the entity manager test, but we have a default plugin manager here, so..

   a) We should just use DiscoveryInterface, not Cached.
   b) getDefinition() on the discovery should *not* be called, so remove that.

2. +++ b/core/tests/Drupal/Tests/Core/Mail/MailManagerTest.php
   @@ -0,0 +1,163 @@
   +    $interface = array(
   +      'default' => 'php_mail',
   +      'mailmanagertest_getinstance' => 'test_mail_collector',
   +    );
   ...
   +    $options = array('module' => 'mailmanagertest', 'key' => 'getinstance');
   

   Super-minor: module/key seems a bit strange, can we use something like example_testkey instead?

Thanks, this looks good I think!

Pro-tip: Do not name your interdiff's interdiff.txt. As you can see, they get renamed to interdiff_xxxx.txt, and the way drupal.org figures out what number to use, it has to loop from 1 to (currently) 2386, checking if the file already exists on every loop. That's not nice.

So isn't this a bug in Drupal/drupal.org that we should consider fixing? :)

Was about to commit...

git ac https://drupal.org/files/issues/core-2187495-68-mail-to-plugins.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 33125  100 33125    0     0  18239      0  0:00:01  0:00:01 --:--:-- 28506
error: core/modules/system/lib/Drupal/system/Tests/InstallerTest.php: does not exist in index

:(

core/modules/system/lib/Drupal/system/Tests/InstallerTest.php => core/modules/simpletest/lib/Drupal/simpletest/InstallerTestBase.php

71: 2187495-mail-plugins-71.patch queued for re-testing.

Thanks for the re-roll, back to RTBC then.

Committed 438494d and pushed to 8.x. Thanks!