Problem/motivation

- MailInterface defines format() and send(). It is a common requirement to use two different implementations for those two methods, see #1135262: drupal_mail() should support using different MailSystemInterface classes for format() and mail(), e.g. format it as a MIME mail and set it out using one of the many mail as a service providers or SMTP.
- The construction of the used mail system implementation is hardcoded and can not be changed.
- The mailsystem module in 7.x currently circuments this limitation

Proposed solution

Move the logic in drupal_mail_system() to a service, keep the function as a wrapper. Then a module like mailsystem can replace the factory and implement something like the following pattern to work around the core limitation in a clean way:

$mail = new MailAdapter($format_mail, $send_mail);

$format_mail and $send_mail could be instantiated separately based on specific configuration, MailAdapter would then forward the calls accordingly.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#13 mailfactory-1889644-13.patch10.01 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,069 pass(es). View
#10 mailfactory-1889644-10.patch9.86 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,118 pass(es). View
#7 mailfactory-1889644-7.patch9.85 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,812 pass(es). View
#7 mailfactory-1889644-7-interdiff.txt6.37 KBBerdir
#2 mailfactory-1889644-2.patch9.74 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,720 pass(es), 73 fail(s), and 9 exception(s). View

Comments

oadaeh’s picture

Berdir’s picture

Status: Active » Needs review
FileSize
9.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,720 pass(es), 73 fail(s), and 9 exception(s). View

This is the minimal implementation that doesn't introduce any API changes.

Completely untested :)

Will update the issue summary to explain why this is useful.

Berdir’s picture

Title: convert mail system variables into container services » Convert drupal_mail_system() to a MailFactory service to allow more flexible replacements

Updating title for the updated focus of this issue

dawehner’s picture

Title: Convert drupal_mail_system() to a MailFactory service to allow more flexible replacements » convert mail system variables into container services
@@ -200,98 +200,21 @@ function drupal_mail($module, $key, $to, $langcode, $params = array(), $from = N
+  Drupal::service('mail.factory')->get($module, $key);
 }
 
 /**

@@ -0,0 +1,134 @@
+    $this->instances = &drupal_static(__FUNCTION__, array());

Do we still want to have a Drupal::static? All you probably need is the property on the factory object ... Hopefully we don't consider drupal_static_reset('drupal_mail_system') as an API.

@@ -0,0 +1,134 @@
+   * An implementation needs to implement the following methods:
+   * - format: Allows to preprocess, format, and postprocess a mail
+   *   message before it is passed to the sending system. By default, all messages
+   *   may contain HTML and are converted to plain-text by the
+   *   Drupal\Core\Mail\PhpMail implementation. For example, an alternative
+   *   implementation could override the default implementation and additionally
+   *   sanitize the HTML for usage in a MIME-encoded e-mail, but still invoking
+   *   the Drupal\Core\Mail\PhpMail implementation to generate an alternate
+   *   plain-text version for sending.
+   * - mail: Sends a message through a custom mail sending engine.
+   *   By default, all messages are sent via PHP's mail() function by the
+   *   Drupal\Core\Mail\PhpMail implementation.

Yes this is just moved, but that is kind of a redundant information as it is part of the interface. There you seem to have way less detailed descriptions.

@@ -0,0 +1,134 @@
+
+} ¶

Additional empty line and whitespace :p

oadaeh’s picture

Title: convert mail system variables into container services » Convert drupal_mail_system() to a MailFactory service to allow more flexible replacements

Resetting @Berdir's title change.

Status: Needs review » Needs work

The last submitted patch, mailfactory-1889644-2.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.37 KB
9.85 KB
PASSED: [[SimpleTest]]: [MySQL] 57,812 pass(es). View

Thanks for the review.

- Removed the drupal_static(), that was a left-over
- Changed the Mail Factory to store the config object and not the actual configuration, that allows to change it while the factory is already instantiated
- Added the missing return in drupal_mail_system(). Going to be hard without that :p
- Moved the format documentation to MailInterface::format(), removed the mail part, not really relevant for either the interface or the factory to document what the default implementation is and does?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This looks really good for me.

@@ -214,7 +214,7 @@ function drupal_mail($module, $key, $to, $langcode, $params = array(), $from = N
 function drupal_mail_system($module, $key) {
-  Drupal::service('mail.factory')->get($module, $key);
+  return Drupal::service('mail.factory')->get($module, $key);
 }

That was just an implementation detail

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The patch looks good to me.

Needs a reroll

git ac https://drupal.org/files/mailfactory-1889644-7.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10089  100 10089    0     0  24819      0 --:--:-- --:--:-- --:--:-- 29500
error: patch failed: core/includes/mail.inc:200
error: core/includes/mail.inc: patch does not apply
Berdir’s picture

Status: Needs work » Needs review
FileSize
9.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,118 pass(es). View

Really? ;)

mail.inc didn't change in *5 month* before that config() stuff ;)

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Just conflicted due to config()/Drupal::config() in the removed code. So I think it's ok to move it back to RTBC.

dawehner’s picture

+1

  1. +++ b/core/lib/Drupal/Core/Mail/MailFactory.php
    @@ -0,0 +1,120 @@
    + * Contains Drupal\Core\Mail\MailFactory.
    

    Theoretically we just add a "\" here :(

  2. +++ b/core/lib/Drupal/Core/Mail/MailFactory.php
    @@ -0,0 +1,120 @@
    +  public function __construct(ConfigFactory $configFactory) {
    

    Needs docs :(

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,069 pass(es). View

Fixed those docs.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks

alexpott’s picture

Title: Convert drupal_mail_system() to a MailFactory service to allow more flexible replacements » Change notice: Convert drupal_mail_system() to a MailFactory service to allow more flexible replacements
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 4d783b7 and pushed to 8.x. Thanks!

Let's can a change notice to announce the new possibilities this patch permits.

Berdir’s picture

Title: Change notice: Convert drupal_mail_system() to a MailFactory service to allow more flexible replacements » Convert drupal_mail_system() to a MailFactory service to allow more flexible replacements
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Update issue summary