Deprecate all of mail.inc, move drupal_mail to method on Mail plugin manager

Nice.

While moving this over, can we also already convert WrapMailUnitTest to a phpunit unit test or should we do that in a follow-up? Seems like it should be trivial now.

Suppose we need \Drupal::mail() wrapper for drupal_mail()

+++ b/core/lib/Drupal/Core/Mail/MailManager.php
@@ -141,4 +160,102 @@ public function getInstance(array $options) {
+  public function hasDefinition($plugin_id) {
+    // TODO: Implement hasDefinition() method.

is this needed?

+++ b/core/lib/Drupal/Core/Utility/Mail.php
@@ -0,0 +1,390 @@
+ * Contains \Drupal\Core\Utility\Mail.
+ */
+
+namespace Drupal\Core\Utility;
...
+class Mail {

why not in Drupal\Core\Mail namespace?
and maybe MailFormatHelper.
It is not a general Utility class, it is very focused on formatting email text

1. +++ b/core/lib/Drupal.php
   @@ -647,4 +647,120 @@ public static function logger($channel) {
   +  public static function mail($module, $key, $to, $langcode, $params = array(), $reply = NULL, $send = TRUE) {
   +    return static::$container->get('plugin.manager.mail')->mail($module, $key, $to, $langcode, $params, $reply, $send);
   +  }
   

   Drupal::mail() is unnecessary. It's very much not an "every day thing that procedural code needs" (which is the only reason to put something in Drupal::)

2. +++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   @@ -0,0 +1,392 @@
   +class MailFormatHelper {
   

   This should be an instantiated object, not a bunch of functions with colons in them. :-) All-static classes are a very bad code smell.

Thanks for dropping the \Drupal::mail() method. Yes, sending mails isn't exactly >80% material in my book.

A possible addition may be discussed in a separate followup issue. But normally, I'd expect consuming code to properly inject this stuff.

Re #12 Why should a value object contain logic about its formatting? Crell is right, also making it a service would allow one to easily override it..i am pretty sure there are already cases in contrib that could take advantage of it.

The value object then could contain format() methods that proxy calls to this service for DX, but i really think that this should be a nice followup

A mail subsystem is composed of discrete components:

1. A Mail message
2. Handler selection + preprocessing (e.g. mailsystem, hook_mail_alter())
3. Formatting engine (e.g. mimemail, drupal_html_to_text())
4. Sending engine (e.g. SMTP/phpmailer)

Merging formatting into the manager would break a clean separation of concerns.

MailFormatHelper is essentially a TextMailMessageFormatter (Content-Type: text/plain).

FWIW, I'd recommend to not over-engineer this. If that's the goal, then we should drop our code entirely and adopt one of the real PHP mail libraries/packages instead (e.g., SwiftMailer, ZendMailer, PHPMailer, etc). (which we should have done a decade ago already)

Re #17
1) There's Message in contact module so current stateless definition could be moved to system or Entity - needs some clean-up
2) entity view modes as we have for rss and search* - done
3) Formaters and filters - done
4) drupal_mail() => service - tbd

probably that means to create a kind of serialization and templating to support special view mode for content entity at least

I know we're very late in the cycle, but sun has a point in #17. If we need a Robust Mail Handling System(tm), aren't there several already that we could composer install and call it a day? (That's what we did for RSS/Atom rather than trying to rewrite those parsers ourselves.)

Can we limit this issue to what we have? Yes, static methods are not that great but this seems to be in line with other utility classes that we added like Xss, String and Unicode.

The class is not a formatter. Just a bunch of helper methods for formatting plain-text mails. We already have format() on MailInterface and while that should be a separate plugin, it currently is not, so that is the formatter. (Mailsystem 8.x in contrib solves this problem by extending the MailManager and an adapter plugin that can forward format() and send() to different classes).

And replacing the whole mail system is really not in the scope of this issue...

I am fine with going on with #13 for now

1. +++ b/core/includes/mail.inc
   @@ -453,80 +212,46 @@ function drupal_html_to_text($string, $allowed_tags = NULL) {
    function _drupal_wrap_mail_line(&$line, $key, $values) {
   ...
    function _drupal_html_to_mail_urls($match = NULL, $reset = FALSE) {
   ...
    function _drupal_html_to_text_clean($indent) {
   ...
    function _drupal_html_to_text_pad($text, $pad, $prefix = '') {
   

   Lets completely get rid of those. Underscore means its private and shouldnt been used directly.
   Which would mean you could convert some of those to be anonymous functions like you proposed before

2. +++ b/core/tests/Drupal/Tests/Core/Mail/WrapMailUnitTest.php
   @@ -2,24 +2,24 @@
   + * Definition of Drupal\Tests\Core\Mail\WrapMailUnitTest.
   

   Since we change it lets make it Contains

3. +++ b/core/tests/Drupal/Tests/Core/Mail/WrapMailUnitTest.php
   @@ -2,24 +2,24 @@
   +class WrapMailUnitTest extends UnitTestCase {
   

   Lets get rid "Unit" from its name. Maybe also rename it to MailFormatHelperTest

4. +++ b/core/tests/Drupal/Tests/Core/Mail/WrapMailUnitTest.php
   @@ -2,24 +2,24 @@
   +  public function testDrupalWrapMail() {
   

   and this method testWrapMail

.

Re-roll of #13 plus fixes for #24.

Looks like I missed the part about "Which would mean you could convert some of those to be anonymous functions like you proposed before". I just removed them. :-(

Missed a call to _drupal_html_to_mail_urls in a preg_replace_callback().

1. +++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   @@ -0,0 +1,392 @@
   +    $clean_indent = self::htmlToTextClean($indent);
   ...
   +      array_walk($lines, '\Drupal\Core\Mail\MailFormatHelper::wrapMailLine', array('soft' => $soft, 'length' => strlen($indent)));
   ...
   +      self::wrapMailLine($text, 0, array('soft' => $soft, 'length' => strlen($indent)));
   ...
   +    if (empty(self::$supportedTags)) {
   +      self::$supportedTags = array('a', 'em', 'i', 'strong', 'b', 'br', 'p',
   +        'blockquote', 'ul', 'ol', 'li', 'dl', 'dt', 'dd', 'h1', 'h2', 'h3',
   +        'h4', 'h5', 'h6', 'hr');
   ...
   +    $allowed_tags = isset($allowed_tags) ? array_intersect(self::$supportedTags, $allowed_tags) : self::$supportedTags;
   ...
   +    $string = preg_replace_callback($pattern, 'self::htmlToMailUrls', $string);
   +    $urls = self::htmlToMailUrls();
   

   Is there a reason we cannot use static::?

2. +++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   @@ -0,0 +1,392 @@
   +   * hello_drupal.docx will produce the following Content-Type:
   +   * Content-Type:
   +   * application/vnd.openxmlformats-officedocument.wordprocessingml.document;
   +   * name="hello_drupal.docx"
   

   I guess we want an @code wrap here.

3. +++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   @@ -0,0 +1,392 @@
   +    global $base_url, $base_path;
   ...
   +        self::$regexp = '@^' . preg_quote($base_path, '@') . '@';
   ...
   +        self::$urls[] = strpos($url, '://') ? $url : preg_replace(self::$regexp, $base_url . '/', $url);
   +        return $label . ' [' . count(self::$urls) . ']';
   

   Do we have a follow up to use the request context instead?

4. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -49,12 +64,16 @@ class MailManager extends DefaultPluginManager {
   +    $this->siteConfig = $config_factory->get('system.site');
   +    $this->logger = $logger_factory->get('mail');
   

   Personally I try to avoid calling anything in the constructor , at least the config factory should be avoided as this could trigger everything, so services which potentially send mails will fetch the system.site config

5. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -141,4 +160,90 @@ public function getInstance(array $options) {
   +      'id'       => $module . '_' . $key,
   +      'module'   => $module,
   +      'key'      => $key,
   +      'to'       => $to,
   +      'from'     => $site_mail,
   +      'reply-to' => $reply,
   +      'langcode' => $langcode,
   +      'params'   => $params,
   +      'send'     => TRUE,
   +      'subject'  => '',
   +      'body'     => array(),
   ...
   +      'MIME-Version'              => '1.0',
   +      'Content-Type'              => 'text/plain; charset=UTF-8; format=flowed; delsp=yes',
   +      'Content-Transfer-Encoding' => '8Bit',
   +      'X-Mailer'                  => 'Drupal',
   

   Can we get rid of the silly spaces?

6. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -141,4 +160,90 @@ public function getInstance(array $options) {
   +          drupal_set_message(t('Unable to send email. Contact the site administrator if the problem persists.'), 'error');
   

   Let's try to use the StringTranslationTrait instead ...

7. +++ b/core/lib/Drupal/Core/Mail/MailManagerInterface.php
   @@ -0,0 +1,126 @@
   +   *       drupal_mail('example', 'notice', $account->mail, user_preferred_langcode($account), $params);
   ...
   +   *
   +   * Another example, which uses drupal_mail() to format a message for sending
   +   * later:
   ...
   +   *   $message = drupal_mail('example', 'notice', $to, $langcode, $params, FALSE);
   ...
   +   *   If TRUE, drupal_mail() will call drupal_mail_system()->mail() to deliver
   

   Can we update this documentation to not refer to drupal_mail any longer?

8. +++ b/core/lib/Drupal/Core/Mail/MailManagerInterface.php
   diff --git a/core/modules/system/src/Tests/Mail/WrapMailUnitTest.php b/core/tests/Drupal/Tests/Core/Mail/MailFormatHelperTest.php
   similarity index 60%
   
   similarity index 60%
   rename from core/modules/system/src/Tests/Mail/WrapMailUnitTest.php
   
   rename from core/modules/system/src/Tests/Mail/WrapMailUnitTest.php
   rename to core/tests/Drupal/Tests/Core/Mail/MailFormatHelperTest.php
   
   rename to core/tests/Drupal/Tests/Core/Mail/MailFormatHelperTest.php
   index e9a6619..7fabc0d 100644
   
   index e9a6619..7fabc0d 100644
   --- a/core/modules/system/src/Tests/Mail/WrapMailUnitTest.php
   
   --- a/core/modules/system/src/Tests/Mail/WrapMailUnitTest.php
   +++ b/core/tests/Drupal/Tests/Core/Mail/MailFormatHelperTest.php
   
   +++ b/core/tests/Drupal/Tests/Core/Mail/MailFormatHelperTest.php
   +++ b/core/tests/Drupal/Tests/Core/Mail/MailFormatHelperTest.php
   @@ -2,24 +2,24 @@
   
   @@ -2,24 +2,24 @@
    
    /**
     * @file
   - * Definition of Drupal\system\Tests\Mail\WrapMailUnitTest.
   + * Contains Drupal\Tests\Core\Mail\MailFormatHelperTest.
   

   nice!

Thanks for the review @dawehner!

1. Fixed
2. Fixed
3. Fixed
4. Fixed
5. Fixed
6. Fixed
7. Might need someone who knows this better to improve the docs. Otherwise, I could find/replace drupal_mail with Drupal::service('plugin.manager.mail')->mail()

Might need someone who knows this better to improve the docs. Otherwise, I could find/replace drupal_mail with Drupal::service('plugin.manager.mail')->mail()

Better than nothing.

Better than nothing.

Fair enough.

Thank you. ( we should write a change record )

Added draft change record https://www.drupal.org/node/2309379

+++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
@@ -0,0 +1,395 @@
+  public static function wrapMailLine(&$line, $key, $values) {
...
+  public static function htmlToMailUrls($match = NULL, $reset = FALSE) {
...
+  public static function htmlToTextClean($indent) {
...
+  public static function htmlToTextPad($text, $pad, $prefix = '') {

Let's make these protected. These are replacing _drupal functions and their only usages are in this class. Less public facing API to maintain. And yes this will work with the callbacks... http://3v4l.org/2XKJo

+++ b/core/tests/Drupal/Tests/Core/Mail/MailManagerTest.php
@@ -85,8 +85,10 @@ protected function setUpMailManager($interface = array()) {
+    $this->loggerFactory = $this->getMock('\Drupal\Core\Logger\LoggerChannelFactoryInterface');

Can we add a property on the class for this or not use $this->

Fixes for #36.

Also added a property for $this->mailManager which was being set dynamically.

Also, minor code style cleanups.

Re-roll

1. +++ b/core/includes/mail.inc
   @@ -5,9 +5,7 @@
   +use Drupal\Core\Mail\MailFormatHelper;
   

   It is a little bit sad that like 95% of that functionality is generic enough to be reused for example in a CLI environment, so we maybe could work on a follow up to provide a more reusable component.

2. +++ b/core/lib/Drupal/Core/Mail/MailFormatHelper.php
   @@ -0,0 +1,395 @@
   +   *
   +   * We deliberately use LF rather than CRLF, see drupal_mail().
   +   *
   ...
   +   * The output will be suitable for use as 'format=flowed; delsp=yes' text
   +   * (RFC 3676) and can be passed directly to drupal_mail() for sending.
   +   *
   +   * We deliberately use LF rather than CRLF, see drupal_mail().
   

   Should we refer to the new code now?

3. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -136,9 +154,100 @@ public function getInstance(array $options) {
   +        throw new InvalidPluginDefinitionException($plugin_id, String::format('Class %class does not implement interface %interface', array(
   +          '%class' => get_class($plugin),
   +          '%interface' => 'Drupal\Core\Mail\MailInterface',
   +        )));
          }
   

   Oh wow, I thought we would have had a generic solution for that already but it doesn't seem to be the case. Would be worth as a follow up in general, as it is quite a DX improvement.

4. +++ b/core/lib/Drupal/Core/Mail/MailManager.php
   @@ -136,9 +154,100 @@ public function getInstance(array $options) {
   +    if (function_exists($function = $module . '_mail')) {
   +      $function($key, $message, $params);
   +    }
   

   Follow up: Allow to use somehow callables here instead.

5. +++ b/core/lib/Drupal/Core/Mail/MailManagerInterface.php
   @@ -0,0 +1,126 @@
   +/**
   + * Defines an interface for the mail manager service.
   + */
   +interface MailManagerInterface extends PluginManagerInterface {
   

   Yeah fine so far, but this line does not tell me what the main manager actually does

6. +++ b/core/lib/Drupal/Core/Mail/MailManagerInterface.php
   @@ -0,0 +1,126 @@
   +   * Sending an email works with defining an email template (subject, text
   +   * and possibly email headers) and the replacement values to use in the
   +   * appropriate places in the template. Processed email templates are
   +   * requested from hook_mail() from the module sending the email. Any module
   +   * can modify the composed email message array using hook_mail_alter().
   +   * Finally drupal_mail_system()->mail() sends the email, which can
   +   * be reused if the exact same composed email is to be sent to multiple
   +   * recipients.
   +   *
   +   * Finding out what language to send the email with needs some consideration.
   +   * If you send email to a user, her preferred language should be fine, so
   +   * use user_preferred_langcode(). If you send email based on form values
   +   * filled on the page, there are two additional choices if you are not
   +   * sending the email to a user on the site. You can either use the language
   +   * used to generate the page or the site default language. See
   +   * language_default(). The former is good if sending email to the person
   +   * filling the form, the later is good if you send email to an address
   +   * previously set up (like contact addresses in a contact form).
   +   *
   +   * Taking care of always using the proper language is even more important
   +   * when sending emails in a row to multiple users. Hook_mail() abstracts
   +   * whether the mail text comes from an administrator setting or is
   +   * static in the source code. It should also deal with common mail tokens,
   +   * only receiving $params which are unique to the actual email at hand.
   

   Just in case you have nothing to do you could wrap them with higher density, so that it is more looking like full justification

4. #1975136: Token::replace() should accept any type of callable, see last comments, $function() actually works for arrays in PHP 5.4+. Happy to see you didn't know this either :)

1. Created follow up #2318277: Move some methods in MailFormatHelper to a generic helper class
2. Fixed
3. Created follow up #2318281: Provide a standard way to check a plugin is an instance of the required interface
4. Berdir responded in #43
5. Fixed
6. Fixed

+    if (function_exists($function = $module . '_mail')) {
+      $function($key, $message, $params);
+    }

srsly? Yes, followup, yadda, yadda, but, rly?

srsly? Yes, followup, yadda, yadda, but, rly?

What I said! berdir just pointed out that it theoretically supports callbacks if you use $foo(), we need still a proper way in the future.

On top of that it seems to be that it makes kinda sense to put a the mail manager onto ControllerBase as well as FormBase and maybe \Drupal

Sending mail is a very rare operation. There's no need to put that on the base classes. It's easy enough to inject when needed.

Committed 6f476b8 and pushed to 8.0.x. Thanks!

Created follow-up #2325955: Refactor MailFormatHelper to allow injection and testing. for the use of globals in a static method.