MailManager::mail() should run inside its own render context: it sends e-mails, not (cacheable) responses

+++ b/core/lib/Drupal/Core/Mail/MailManager.php
@@ -157,6 +169,20 @@ public function getInstance(array $options) {
+    return $this->renderer->executeInRenderContext(new RenderContext(), function() use ($module, $key, $to, $langcode, $params, $reply, $send) {

Quoting myself (more or less) from #2626298: REST module must cache only responses to GET requests:

Apparently we have to use a class marked as @internal. If this really is a thing anyone is expected to do, we have to remove @internal.

RenderContext is.

That's a unit test with a custom constructor? Pretty obvious to me that that needs to be updated, not sure how that could pass for you? Make sure you run it with phpunit directly.

Let's try again.

IS pretty please.

Closed duplicate #2744487: Rendering content too early error from user_mail_tokens

It happens when trying to mail messages with tokens which contains rendering (for example URL [user:one-time-login-url]) in REST endpoint.

#11 works for me!
Thanks a lot!

Added a simple unit test, I think that is sufficent for testing this, as we know that sending mails still works? We'd need a test mail that triggers cacheable metadata and then do that somewhere in a controller that returns a response object to have a full test of the problematic scenario.

Also updated the issue summary.

Looks great! docs updates are not really in scope but awesome

@andypost: If you are referring to doMail() then that is in scope as the method is added here. I just copied the docs from the mail() interface and removed the long description.

If you refer to the removal of @internal on RenderContext then yes, that is partially off topic. But I'd argue that we're technically not allowed to use it here as long as it is tagged as internal as it is a different namespace so you could say it is a blocker for doing this :)

Thanks for RTBC

Looks great and along with a couple of other issues should get rid of most early rendering exceptions.

Committed ce31c4e and pushed to 8.2.x. Thanks!

About 8.1.x, I thought I wrote that in my comment above but apparently didn't.

We're changing the constructor of a service. And there are contrib modules ( at least one: mailsystem) that override that, and any site that is using them and updates to this without also updating mailsystem will be completely dead until they update the mailsystem code as well.

That is the risk of doing changes like that and I'll have to figure out how to deal with it in mailsystem: #2748757: Update MailsystemManager::__construct() for 8.2.x compatibilty. I still think that allowing changes like that is a bigger problem than we'd like to acknowledge but that's a different topic :)

AFAIK, we're not allowing such a change in a patch update and it would complicate updating core even more for users (who expects that a core patch update requires contrib updates to keep your site functioning?).

About the internal, but isn't that the very definition of @internal? You're not allowed to use it outside of the component where it is defined. PHPStorm treats this the same as a deprecated usage (it is striked through). You also agreed in comment #8.

Hi, drudes!
Thanks for fixing this, but here is my story.
A month ago I've started to port drupal-netsmtp module - smtp mailer using PEAR::Net_SMTP.
So I removed settings overriding mechanism for detecting mailer and formatter per key/module/etc and delegated this to the Mailsystem manager.
Also I've added a travis ci to test a ported module, and a couple of days ago I've decided to add matrix build for current drupal version on which travis should perform testing.

I was really surprised when tests failed on drupal 8.2.x because some wrong arguments were passed to Mailsystem manager constructor.
Why is this matter to me? Well because while current stable release is 8.1 I wouldn't even be able to know (till that moment with updating my travis env vars) that something could be wrong with drupal minor version in conjunction with other contrib.

As a developer (regarding http://semver.org/ ) I don't expect that minor versions can change method signatures. At least there should be created a definite change record (CR) before change will be committed.
Anyway I'll leave here my draft to this CR, maybe it will be informational for someone https://www.drupal.org/node/2760303

Yeah, I know, it is unfortunate. See https://www.drupal.org/core/d8-bc-policy why this is allowed.

Also, if you use mailsystem 8.x-4.x-dev that is already fixed and works with 8.1 and 8.2. Please create an issue for me as reminder to release a new version so that people use that by default and are prepared.

Yea, Berdir, I will do.