Core Mail class prints non-overridable errors on the page

+1 for it being the callers job

So now the mail hanlder just need to check result of $this->mailManager->mail()

@return array
* The $message array structure containing all details of the message. If
* already sent ($send = TRUE), then the 'result' element will contain the
* success indicator of the email, failure being already written to the
* watchdog.

Given the point of the release we're at we should be handling this in the most non-disruptive way possible. It should be possible to disable the message and have the calling code handle it but the default should be the set the message. We could even deprecate this for Drupal 9 and say that we going to remove this option in Drupal 9 and make it 100% the callers responsibility.

How far is this from being ready to be committed? Does look like a beneficial addition in many use cases.

Here's a patch. One thought is should we deprecate the behaviour and thereby not use it in core?

New patch reroll and fixes tests

Adding an argument to mail() affects derived classes that override the function and will not have the extra argument. In particular, this applies to MailsystemManager in the popular Mail System module.

Fatal error: Declaration of Drupal\mailsystem\MailsystemManager::mail($module, $key, $to, $langcode, $params = Array, $reply = NULL, $send = true) must be compatible with Drupal\Core\Mail\MailManager::mail($module, $key, $to, $langcode, $params = Array, $reply = NULL, $send = true, $display_error = true) in XXX/web/modules/mailsystem/src/MailsystemManager.php on line 20

I'm not an expert, but my guess that this change is allowed by the BC rules. However we should have a CR to flag the impact on other modules.

1) It seems like a good idea to deprecate the message from this function to allow the caller to make the correct presentation rather than printing a generic one.

But how will deprecation work in the current patch? Shall we trigger a deprecation warning if $display_error = TRUE? This forces callers to fill in all intermediate optional parameters so it's not a simple edit. Then in D9 we remove $display_error from the interface and callers remove all the parameters they just added? However any unchanged calling code runs in D9 just the message is missing. It doesn't seem that neat!

Alternative idea: create a new method MailsystemManager::sendMail with identical signature to mail, however it doesn't output a message. Deprecate mail and remove it in D9. Now calling code has a simple change now from mail to sendMail and no need to change again in D9. Unchanged calling code will fail in D9.

2) I wonder if we should also remove the generic log? Again it might not be appropriate.

• If simplenews module is sending a newsletter to 100k subscribers, and there is a config fault blocking sending, we don't want 100k logs. simplenews can make a single combined log.
• An error log is not necessarily appropriate. Alternative mail-sending plug-in implementations (e.g. Swiftmailer sending over SMTP) can return false much more easily that the default PHP mailer, and in particular might do so for a bad email address. (True, the API specifies return "if the mail was successfully accepted for delivery" but the underlying library doesn't have a concept of "accepted for delivery"). If a bot has created an account with a bogus email address then we don't want an error log each time.

==

Setting "Contributed project blocker" for simplenews module.

Update on #18

After thinking some more, I think it's best to defer the deprecation until D9. If we try to do it now, we need to alter all calls to MailManager->mail() in core to set $display_error = FALSE (filling in all intermediate optional parameters) and then remove all of that code again in D9. I raised #3053749: Show specific/appropriate messages for mail errors.

In terms of the log, let's leave that to a separate issue, I don't think it's quite the same situation.

Updated patch adds a @todo for the deprecation.

I agree: the deprecation is tricky whereas the fix here is easy.

This is definitely an API change, we can not add new arguments to an existing method, and removing it would be an API change again.

We have a $params array that is typically used to pass stuff through to hook_mail(), so we could easily define a special key foy like '_display_error' that we'd use that for, that's also easy to remove again.

Another option would be to not leave BC to the caller but make it a site-wide thing with a setting, we've done similar things before e.g. in the serialization module with returning (or not) values in the correct type. Introduce a setting, false by default, set it to TRUE in an update function and a requirements hook.

Or last, say that displaying a message or not is not actually covered by BC policy, which I think would be fair, but we'd need buy-in from a maintainer, not sure if framework or release :) It's certainly less of an API change than the current patch. Either way, we should actually revisit why this was originally added, which was a long time ago in #169627: Proper e-mail errors logging apparently. On example could be a site that requires approval of registrations, and if the admin wouldn't get that message, he'd not know about the new registration, so we tell the user that he might want to contact the admin directly.. So maybe we should somehow keep that behavior for specific use cases and display a context-specific message?

This is definitely an API change

It's the change to the interface that would break contrib implementations of MailManager right? I'd forgotten that, I thought we were safe because the new parameter was optional.

We have a $params array that is typically used to pass stuff through to hook_mail(), so we could easily define a special key foy like '_display_error' that we'd use that for, that's also easy to remove again.

That seems a good solution.

On example could be a site that requires approval of registrations, and if the admin wouldn't get that message, he'd not know about the new registration, so we tell the user that he might want to contact the admin directly.. So maybe we should somehow keep that behavior for specific use cases and display a context-specific message?

What @larowlan said in #3 makes sense to me: ideally (leaving aside BC) in the future error displaying should be the responsibility of the calling context, not MailManager.

The convenience to callers of handling error display inside MailManager seems too small to be worth the complexity.

> What @larowlan said in #3 makes sense to me: ideally (leaving aside BC) in the future error displaying should be the responsibility of the calling context, not MailManager.

Absolutely. But it's still *our* responsibility for the calls that are happening in Drupal core, so if we remove it by default, we need to think about which callers need to do something about it now. That's what I mean.

Of course, as long as core itself still displays the error that would be OK, so in that regard, the current patch would be OK. But the policy that we have in Drupal core is that we are not allowed to rely on deprecated code usages/behaviors. But the more I think about it, we should IMHO either not have BC at all for the message or do it with a global flag or so. If we have a flag, then by our policy, we would need to do a @trigger_error() to make every caller aware that he is relying on a deprecated before, only then to invert it and do a @trigger_error() in 9.x for anyone passing the no longer existing key, so forcing anyone using mail() to change their code twice.

If we have a flag, then by our policy, we would need to do a @trigger_error() to make every caller aware that he is relying on a deprecated before, only then to invert it and do a @trigger_error() in 9.x for anyone passing the no longer existing key, so forcing anyone using mail() to change their code twice.

What do you think of Adam's idea of deprecating mail() and replacing it with a new method sendMail()? This way we preserve BC but everyone using mail() only has to change their code once.

BC and the interface goes in both directions. Adding a new method would also break contrib implementations unless it would internally call the same method again and set a flag or something.

I still think the easiest option is to just remove it or maybe have a setting but IMHO that's overkill.

Thanks @Berdir

So we remove the internal error message in MailManagerInterface->mail() and add messages to all calling code in core. I have updated the IS.

I don't see any advantage to a global setting to control the internal error message. Calling code in core will print error messages. Calling code in contrib maybe some prints and some doesn't. So if you have the setting on, then you definitely get duplicates and if you have it off you maybe get some missing messages.

I'm not convinced we can remove the message in a minor release and get an acceptable result. Suppose we update calling code in contrib to print a message. Then if someone is still running against the old core, there will be a double message. Or if they upgrade core before contrib there will be no message.

@Berdir #22

but we'd need buy-in from a maintainer, not sure if framework or release :) It's certainly less of an API change than the current patch. Either way, we should actually revisit why this was originally added, which was a long time ago in #169627: Proper e-mail errors logging apparently

Yes so there are lots of complications to worry about.

@Berdir #22

We have a $params array that is typically used to pass stuff through to hook_mail(), so we could easily define a special key foy like '_display_error' that we'd use that for, that's also easy to remove again.

This seems like a good idea - why don't we do that? Then it can be almost like the existing patch in #20. We can have the same deprecation strategy - to alter the default in D9 and change core callers at the same time.

Well, one argument against doing that is what I wrote in #25:

> If we have a flag, then by our policy, we would need to do a @trigger_error() to make every caller aware that he is relying on a deprecated before, only then to invert it and do a @trigger_error() in 9.x for anyone passing the no longer existing key, so forcing anyone using mail() to change their code twice.

If we do deprecate it, then core must not in any way rely on it, and we have to basically change it twice and trigger deprecations twice.

One workaround for the double message thing would be to display the identical message, then drupal automatically deduplicates them.

I'm not sure what's best, but I think it would make sense to try and get some input from one of the core maintainers on this, we could bring it up in #d9readiness for example.

So let's drop the idea of deprecation:

• It's not the objective of this issue.
• As you pointed out, the message was carefully added in #169627: Proper e-mail errors logging, specifically because the calling code didn't reliably write code for an error message.

Instead this issue can keep the message by default but add a flag to disable it.

However even then it's tricky. If contrib code sets the flag to request no error, how does it know if the request was followed? There might still be an error displayed if running D8.7 core or for an alternate mail manager implementation that has not implemented the flag.

So instead I propose we add a parameter $params['_error_message'] which is the alternate preferred error message in case of error. If this is an empty string then the error is skipped. If the parameter is missing, then the default error message is used. If new contrib is running against old core then the parameter is ignored, but you still get precisely one error message, so I think it's good.

What you're saying is basically "Permanently keep the current situation of displaying errors as the default permanently, just add a param flag to allow overriding that behavior".

Seems like a good compromise to me.

Basically yes. But nothing is "permanent" - this issue is keeping the current situation of displaying errors as the default. Anyone who wants to tackle the challenge of changing the default is free to do so in a separate issue.

+++ b/core/lib/Drupal/Core/Mail/MailManager.php
@@ -203,6 +203,8 @@ public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL,
    *   (optional) Parameters to build the email.
+   *   The key '_error_message' has the special meaning to specify a string
+   *   to use as an error message if needed.

+++ b/core/lib/Drupal/Core/Mail/MailManagerInterface.php
@@ -103,6 +103,8 @@ interface MailManagerInterface extends PluginManagerInterface {
    *   (optional) Parameters to build the email.
+   *   The key '_error_message' has the special meaning to specify a string
+   *   to use as an error message if needed.

The comment is not flowing correctly. The first line can go up to 80 characters. Also I don't think we should say string here. Ideally we want people to pass translatable markup in.

Thanks @alexpott

I updated the IS to match latest status. I removed "Needs framework manager review" because that was for the tricky BC that we have now avoided.

If this gets to RTBC then I will write the CR.

+++ b/core/lib/Drupal/Core/Mail/MailManager.php
@@ -203,6 +203,8 @@ public function mail($module, $key, $to, $langcode, $params = [], $reply = NULL,
    *   (optional) Parameters to build the email.
+   *   The key '_error_message' has the special meaning to specify translatable
+   *   markup to use as an error message if needed.

We can still join up the documentation. I.e. there is no need for the line break in between email. and The key

Ah sorry I see now.

The key '_error_message' has
+   *   the special meaning to specify translatable markup to use as an error
+   *   message if needed.

Perhaps
Use the key '_error_message' to provide translatable markup to display as a message if an error occurs, or set this to false to disable error display.

Fail is a random: #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest

RTBC except for Needs CR.

CR ready for review.

Committed 5614f82 and pushed to 8.8.x. Thanks!

Not backporting to 8.7.x because this requires implementations to change to support the new array key.

Great thanks @alexpott