Symfony\Component\Mailer\SentMessage is not quite friendly for asserting HTML emails. In functional tests you will likely fetch the original message. I wonder if it is better to capture the original message instead?
True. This MR allows access to either of them. getEmails() returns the original Email objects, while getCapturedMessages() returns a list of SentMessage.
Thanks for #44, it puts the change nicely into a broader perspective.
The front controller (especially the main entry point index.php) could be viewed as application code. It would be great if subsequent refactoring could be made without changing it anymore. It might be possible to achieve this by using the following pattern:
return static function (AppContext $context) {
return new DrupalKernel($context, require 'autoload.php');
};
The AppContext value object could start out as a very basic class only containing $environment = 'prod'. The DrupalKernel constructor can be changed to accept string|AppContext as its first parameter. If the caller passes the environment string, it is used as is, if the caller passes AppContext, the environment string is taken from there. Nothing else.
Then we can figure out the rest in subsequent refactorings without having to touch the front controllers anymore.
Regarding #45 and putenv: If that code path is called exclusively on the CLI, then this is fine.
Regarding $_SERVER and $_ENV: if it is unacceptable to drop $_ENV, then at least ensure that $_SERVER is used as the primary data source and use $_ENV only as a fallback (like its done in the symfony/runtime component):
% git grep '\$_SERVER.*??=' src/Symfony/Component/Runtime/
src/Symfony/Component/Runtime/Internal/autoload_runtime.template:if (is_string($_SERVER['APP_RUNTIME_OPTIONS'] ??= $_ENV['APP_RUNTIME_OPTIONS'] ?? [])) {
src/Symfony/Component/Runtime/Internal/autoload_runtime.template:$_SERVER['APP_RUNTIME'] ??= $_ENV['APP_RUNTIME'] ?? %runtime_class%;
src/Symfony/Component/Runtime/SymfonyRuntime.php: $_SERVER[$envKey] ??= $_ENV[$envKey] ?? 'dev';
src/Symfony/Component/Runtime/SymfonyRuntime.php: $_SERVER[$debugKey] ??= $_ENV[$debugKey] ?? !\in_array($_SERVER[$envKey], (array) ($options['prod_envs'] ?? ['prod']), true);
src/Symfony/Component/Runtime/Tests/phpt/autoload.php:$_SERVER['APP_RUNTIME_OPTIONS'] ??= [];
The php ini value for variables_order is set to GPCS by default in many (most?) Linux distros. That means that environment variables are not parsed into the $_ENV superglobal in many cases. They are still available from $_SERVER though.
The getenv() and putenv() C functions used by the PHP function with the same name are not thread-safe. As a result, concurrent access in multi-threaded environments may lead to hard to debug problems. There have been reports about this in the past (e.g. vlucas/phpdotenv#76 or laravel/framework#7354).
I suggest to try to avoid $_ENV, getenv and putenv and exclusively rely on $_SERVER as an environment variable source - at least initially.
Made the follow-up: #3553063: _update_message_text() calls Url::fromRoute without options → .
Thank you. I left a comment in the merge request. There is also an unresolved suggestion by @smustgrave from a couple of weeks ago. That should be resolved as well.
Setting to needs work for the @todo and the follow-up (see MR comments).
Thanks for taking a look @zengenuity.
There are some docs in the MR (mailer.api.php).
The 3539651-email-content-yaml-plugin-core-emails branch shows how the core emails call sites and implementations might look like with this approach applied.
But neither of those really explain the design choices very well.
Adding dependency evaluation → section and required issue tags.
znerol → changed the visibility of the branch 3539651-all-in-one-integration to hidden.
znerol → changed the visibility of the branch 3539651-introduce-email-yaml to hidden.
I think that this happens only on routes where the default (html) format is disabled.
I am seeing this with (custom) content negotiation. I think a request is affected when it goes through some code path which calls Request::setRequestFormat() (see NegotiationMiddleware::handle()).
I think it isn't necessary to use reflection in this case. Instead I suggest to following:
- Add the
authentication_collectoras an additional dependency to theauthentication_subscriber - Loop through
getSortedProviders()and callapplies()on each of them - Return the provider id of the first match
I pushed a test which asserts that http_kernel.basic is not initialized when http_kernel is retrieved from the container.
Also rebased and converted phpunit annotations to attributes.
I took the opportunity to host a spontaneous bar camp session at DrupalCamp Ruhr. The idea was that participants talk about their projects and tell their success stories and pain points with the current mix of core, contrib and custom extensions around email. Around 15 people took part in the session. Notes by @znerol.
Session format: You talk, I write.
Highlights
- One person operates a big multi-tenant system featuring the DSM+ and the SM module. This project is using the DSM+ mailer policy module and also hooks/event subscribers for routing and customization. Another useful extension in the mix is the Symfony Mailer Log module. It logs each success or failure and also allows to review the rendered emails.
- One person mentiones the Mailsystem module since it permits flexible routing, theming, sending with multiple accounts.
- One person prefers to apply customization directly in the TWIG template for custom projects (including translation).
Pain Points
- Infamous "Recursive rendering detected" in simplenews.
✨
Add setting to disable per-user rendering
Active
This project might just be switched to external newsletter software to avoid this kind of issues. - Sending mails with attachments.
- Multiple smtp accounts. User emails from normal mail account. Newsletter email from scalable infrastructure. Multi tenancy.
- Competing solutions (DSM+ vs. DSML vs. swiftmailer)
- Configuration (for users its config, for other type of mail its different)
hook_mail: What to put here? The bare minimum (like commerce) or most of the logic (like contact).- LocalGov Drupal: Complicated templating, notifications, newsletters, tokens without knowing what the underlying transport will be (DSM+ vs DSML).
- Rendering URLs outside a request-response cycle - needs crude hacks when mails are rendered asynchronously (e.g. inside symfony messenger queue).
- Tokens in mails seem strange and dated.
- User password-reset tokens are implemented in a weird way.
Ideas
- Mailframe module: Receiving (imap) with multiple accounts. Bounce management, Ticketing system (should be solved with Mailframe).
- Mail composing (templates) mailjet language mjml, inki (foundation for email).
- Read receipt handling: E.g., ensure that a contact mail is received by staff.
Questions
- What is the trajectory of core mail?
- What is the plan to deprecate
hook_mail/hook_mail_alter? When and how?
This isn't relevant anymore. There is some test coverage in Drupal\Tests\Core\UnroutedUrlTest and the external option was completely removed from Drupal\Core\Routing\UrlGenerator in
#2575869: Remove all remaining usage of deprecated UrlGeneratorInterface::generateFromPath() and delete it →
.
Reviewed this in context. Adapted the issue title and the CR to match the actual implementation.
There is a slight risk that the newly added create() method in PluginBasebreaks existing contrib/custom plugins with an incompatible create() method in the same way as MenuLinkMock in this issue. Authors of affected plugin classes may rebase onto Drupal\Component\Plugin\PluginBase in that case.
- We are trying to avoid a namespace conflict with an existing contrib module. Claiming the namespace of another existing contrib module doesn't seem to make much sense.
- The core mailer module isn't supposed to stay in core. It is a temporary container for a bunch of service definitions. The service implementations themselves are added to
Drupal\Core\Mailernamespace directly. When enough of the implementation is stable, the service definitions will be moved tocore.services.ymland the core mailer module goes straight from experimental to deprecated.
So, I'd suggest to either agree on a namespace which is available or just not do anything at all.
I like the suggestions in #50. I'd probably prefix the method with create which is common for factory methods. E.g., createAutowired() or createAutowiredInstance().
Mailer module namespace conflict
The experimental core mailer module namespace conflicts with a preexisting contrib mailer project. The participants expect that the transition from experimental to a stable implementation (making the module superfluous) could take a couple of releases. The current draft MR which proposes to introduce a mailer_experiment module isn’t ideal, especially when the module approaches its end state. Members of the group proposed mails or email as an alternative. The group decided to try it with mailer_symfony instead.
It was briefly discussed how the transition from experimental to stable could be performed. One way would be to introduce a new mailer_legacy module containing legacy services (mail manager) in the same release the new mailer becomes stable (and default). Existing sites which haven’t enabled mailer_symfony yet will enable mailer_legacy during the upgrade and continue to use the old API for core mail.
Backwards compatibility
The group briefly touched on backwards compatibility. When the core mailer becomes stable, a lot of modules either need a complete rewrite or will become obsolete. In order to help with that, it's important to provide specific migration documentation. E.g., modules which implemented hook_mail_alter before in order to add a note to every outgoing mail should implement hook/event/whatever. etc.
Also when the core mailer becomes stable, multiple contrib Symfony Mailer integration modules will become obsolete. It is also important to provide documentation on how to migrate from them.
Sending emails defined by another module
In some version of the MR in the template/controller draft issue, the call site looks quite complex. Members of the group point out that this could be a problem. E.g., A custom project which needs a button which (re)sends a password reset or an order receipt, would need to copy and maintain the call site. If that piece is complex, then that means more technical debt for a custom project.
The group discussed whether sending emails defined by another module really should be a supported use case. Projects like ECA would benefit from that.
On the other hand, if the module itself decides whether or not to provide a dedicated API for notifications, then that could include other channels as well. E.g., users might want to receive password resets via an instant messaging app.
There are also types of mails where it isn’t obvious why other modules would want to trigger them. One example is the update status notification. Exposing that as a public API doesn’t seem desirable.
The group doesn’t take a decision whether to go one or the other way. Whether or not to expose triggering an email as a public API may end up being the decision of the email sending module.
Altering emails defined by another module
The group agrees that it is desirable to provide a registry of all emails available on a site. This should also include very simple emails. In that case, the altering module might not see any parameters but only that a message (subject, (un-)rendered body) of a certain module/key combination is about to be sent.
Access to the Symfony Email object
The group discussed which role the Symfony Email object should be playing in the upcoming API. Several members pointed out that Symfony has a history of introducing breaking changes in a rhythm which is difficult to keep up in Drupal projects (this is especially true for contrib modules which are expected to support multiple core versions).
On the other hand, direct access to the Symfony Email object allows for advanced use cases.
One option to tackle this dilemma is to ensure that developers do not need to interact with the Symfony Email object when performing standard email building activities.
Next meeting:
Next planning meeting takes place October 1st at 4PM UTC+2.
I honestly think that the current way routes are defined is really not great DX. [...] I think we shouldn't try to reuse terminology like defaults and controller for this. IMHO, it makes it harder to use, not easier.
This is good to know. I cannot really tell.
plugins are pretty much expected to have a class property in the definition.
True. In a previous version, the class was set to the Symfony\Component\Mime\Email. We do this for constraints, but I guess that this architecture isn't something we want to push more. It also required a custom factory - and I removed it again. The Symfony Email is a value object, it doesn't really map well to a plugin instance.
I think there is a way how plugins could have an interface for callers and still support dynamic arguments for the implementation. The methods which currently make out the controllers could be folded into plugin implementations. The argument resolver could be moved from the renderer to a shared plugin base. I guess I need to try that out next.
I do not think that mail is abandoned. At least, the mail.info.yml declares compatibility with Drupal 10.
https://git.drupalcode.org/project/mail/-/blob/8.x-1.x/mail.info.yml?ref...
What about Core Mailer Preview?
Without rename:
Experimental Mailer:
Core Mailer Preview:
My intention was to only change the module namespace. But of course, if there are two modules available with the same human readable name, that would be confusing.
Thank you! I left one small suggestion.
I do not need help with that. Thank you.
Added a draft MR with an Password Argon2 module. This is implementing the idea from #32.
Thank you.
I was wondering why only the content block was converted and found the answer in the issue comments. Added that info to the issue summary.
Left a question and a suggestion in MR comments.
IS update.
@catch that would basically reverse the outcome from the security team triage call (#24). I'd rather try to go into a direction which doesn't send us running around in circles.
Agree with #43 regarding the UX. But I think we could mitigate that for most sites with #3530186-32: Introduce kernel parameters for password hashing algorithm and options → .
The idea there would be to add a new password_argon2 core module. People still have to manually enable a module, but this is likely a lower barrier than having to add the correct string somewhere in a yaml file.
larowlan → credited znerol → .
Fixed a misnamed variable. The approach in 📌 Extract _user_mail_notify() into a user MailHandler Active changed a little bit (internal service, no interface). Let's see how that goes over there and adapt here if necessary.
Opened an MR that renames mailer into mailer_experiment. In my eyes that name (a) underlines that the module will go away when the experiment is over and (b) it works as a feature flag.
(Although surprisingly with a setup like in #17 I'm finding I'm still logged-in after way more than 30s, will keep playing.)
You might be running into the edge case documented near the end of #17. If there is only activity in one tab from one user, then the session is not garbage collected.
My observations suggest that the behavior is as follows:
- The
timestampcolumn on the session record in the database is refreshed when the site is used (provided that the user hits an uncached route from time to time and provided that this happens in a time window between$settings['session_write_interval']andgc_maxlifetime#17 - The expiry date on the cookie is never refreshed. #18 and #19.
If that is true, then I'd recommend to keep cookie_lifetime values high and only reduce the parameters which govern the session garbage collection for sites which require expiration of idle sessions.
#37 links to the wrong CR. I think it should be that one → . And the CR links back to the wrong issue.
Also left a couple of comments in the MR.
As potential errors would occur when updating Drupal Core and not mailer we can't expect people to have read the documentation / release notes of the mailer module.
The contrib mailer module doesn't have a D11 compatible release. So sites using the contrib mailer wouldn't be able to update to Drupal 11 anyway yet. The experimental core mailer module will not be backported to any Drupal 10 release.
Thanks @stijn.blomme for your thoughts.
Do nothing
If the plan is to only keep the core module in experimental phase, and then move the code to the core framework, this could potentially never impact any website. If the users of the contrib mailer module keep the experimental core module disabled and do not use the new functionality before frame work inclusion.
This is more or less what I tested in #4. The important thing to realize here is that users of the contrib mailer module do not have the choice to enable or disable the experimental core module. Modules need to have a unique name. And it looks like a module from contrib seems to override a module in core with the same name.
But honestly, as a site owner I wouldn't want to risk having two modules with the same name in my source tree. The findings in #4 should be treated as anecdotal. This certainly doesn't proof that two modules with the same name will coexist peacefully over the lifetime of a site.
Thanks to the recent discussions I think I'm starting to understand why @adamps and me are seemingly unable to agree on the broader architecture. I'll try to summarize what I believe are the two approaches, and I'll try to do that without any judgement. I believe the biggest difference lies in the scope of the call site.
Agent model
When sending a message, the call site gathers the minimum amount of information necessary to start the process. For a password reset, this would be the user entity exclusively. For a contact message, this would be the message entity, and only that. This information is passed on to an agent which knows how to build the body and subject, to whom to send it and in which language. The agent also decides who else is getting a copy and where replies are going to end up.
A message sending module provides a default agent for each type. The agent can be replaced with a custom one (satisfies [D2 CAN_REPLACE]). Core ensures that it is easy to implement contrib and custom agents with consistent behavior. I.e., it should be straight forward to create a custom agent which invokes the same hooks / fires the same events as the default one. This could be implemented with an abstract base class (satisfies [D3 CAN_ALTER]).
The agent model moves responsibilities from the call site into the agent implementation. E.g., all of the responsibilities of _user_mail_notify() will end up in some user mail agent. Also all of ContactMailHandler will end up in a contact mail agent. Same for the commerce OrderReceiptMail.
Controller/Template model
When sending an email, the call site gathers all the information necessary to build and send the message. E.g., to build a password reset, this includes the user entity, the fully qualified password reset url and the language. This information is passed on to a rendering component which invokes a controller and a template to generate the message subject and body. Finally, to send the password reset email, the call site gathers the account email address and sets it into the To field and also populates the configurable repylTo address.
An email sending module may provide any combination of default template and controller (also neither or both) for each type of message. An email sending module may also choose to reuse any combination of existing generic controller and existing generic template. The controller/template can be replaced by a custom one (satisfies [D2 CAN_REPLACE] - on the presentation level which is less comprehensive than what the Agent model provides. In order to replace other parts of the message - e.g. addresses, a module would need to implement multiple alter hooks or transport events). The rendering component invokes the same hooks / fires the same events for every type of email (satisfies [D3 CAN_ALTER]).
The controller/template model encourages separating the responsibilities between the call site (gathering the necessary data, populating addresses and other headers) and the controller/template (populating email subject and contents). It doesn't enforce it though.
Also the controller/template model operates on a lower abstraction level than the Agent model.
I tried what happens on a site where the contrib mailer module is enabled during an upgrade. The contrib mailer module doesn't have a Drupal 11 release ready, so I had to patch it.
This is what I did:
Install Drupal 11 and drush:
composer create-project --no-interaction "drupal/recommended-project:^11" mailer-contrib-test
cd mailer-contrib-test
composer require -W drush/drush
Install the contrib mailer and patch for D11 compatibility:
composer require mglaman/composer-drupal-lenient
composer config --merge --json extra.drupal-lenient.allowed-list '["drupal/mailer"]'
composer require 'drupal/mailer:^2.0@beta'
sed -i 's/^core_version_requirement.*/core_version_requirement: ^10 | ^11/' web/modules/contrib/mailer/mailer.info.yml web/modules/contrib/mailer/modules/mailer_example/mailer_example.info.yml
Run a database and install the site:
podman run -d --rm --network=host --name=drupal_memory --tmpfs=/var/lib/pg/data -e PGDATA=/var/lib/pg/data -e POSTGRES_PASSWORD=postgres -e POSTGRES_USER=postgres -e POSTGRES_DB=drupal docker.io/library/postgres:16
./vendor/bin/drush site-install --db-url=pgsql://postgres:postgres@localhost/drupal --no-interaction
Setup mailpit as sendmail binary:
chmod u=rwX,go=rX web/sites/default web/sites/default/*
echo "sendmail_path = ${HOME}/bin/mailpit sendmail" > web/sites/default/php.ini
export PHPRC="${PWD}/web/sites/default"
Enable the contrib mailer and the example module, generate login link, run the built-in webserver:
./vendor/bin/drush en mailer mailer_example
./vendor/bin/drush uli --no-browser --uri=http://localhost:8888
http://localhost:8888/user/reset/1/1755891039/btjHfkrOCd7x1nKu8KtRJ8DmHoNCpwxW548Ul0HLyJk/login
./vendor/bin/drush rs
Open the test page and mailpit in a second tab and confirm that mailer test mail gets delivered:
xdg-open 'http://localhost:8888/admin/config/system/mailer/example'
xdg-open 'http://localhost:8025/'
Upgrade the site to 11.x-dev:
composer require -W "drupal/core-recommended:^11.x-dev" "drupal/core-recipe-unpack:^11.x-dev" "drupal/core-project-message:^11.x-dev" "drupal/core-composer-scaffold:^11.x-dev" "drupal/core:11.x-dev"
./vendor/bin/drush updb
./vendor/bin/drush cr
Confirm that now there are two mailer modules in the filesystem:
find . -name mailer
Open the test form and do the test again.
Interestingly in my case after all these steps, things are still working as before. So, it looks like the contrib mailer module wins the race for the \Drupal\mailer namespace.
#16 touches on things which seem to be more the topic of #3534136-51: Requirements and strategy for Core Mailer → (answer is over there).
The comment over in #3539651-16: Introduce email yaml plugin and email controllers → raises interesting questions. Instead of answering directly, I'm going to rephrase this a little bit into a q/a format. Also I post it here, because it touches on strategy and architecture. I hope that this brings some clarity into the issue.
Q: Looking at the {module}.email.yml file, it seems that the defaults dictionary only contains a _controller key most of the time. Why is it a dictionary in the first place? And is there a situation where more / other key pairs could be useful?
A: The defaults dictionary contains the default attributes for a certain email. The call site may override those using the $params key when calling EmailRenderer:render. So, basically the defaults could specify additional defaults like, e.g., a view_mode to pass to a generic controller which is capable of generating an email from any entity.
Q: Ok, so there could be a generic controller for entities. How would that look like in the {module}.email.yml file?
A: Maybe something like this (not supported yet, but not difficult to implement):
my_module.my_email:
label: 'My custom email'
key: my_email
defaults:
_entity_view: 'entity_test.full'
Some hook_email_info_alter implementation would, extract the entity type and view mode, add them as new defaults and also add a suitable _controller.
Q: How would that work with attribute based plugins?
A: That could be implemented using traits or (abstract) base classes.
Q: What if the _controller is missing and no hook_email_info_alter cares to add one?
A: In that case, the email is rendered by just passing all the arguments to the template. For simple notification style emails its enough to specify an email in a custom module and add a template to the theme.
Q: Emails without "controllers", would that work with attribute based plugins as well?
A: Yes. Such plugin classes would probably end up being nearly empty.
Q: Does the controller in the controller design have the power to set a subject? Or a recipient? Or a custom header?
A: Yes, the controller has access to the Symfony email instance. It can set the subject (and if you look at the MR and the test classes, it does in most cases). And of course it runs in an isolated render context in the switched environment.
Q: In that case, the controller could control any aspect of the email?
A: Yes, it could. It has no opinion on where that stuff should happen and it doesn't force anything. As a consequence, contrib and custom developers can really do whatever they please.
Q: Some people seem to prefer attribute based plugins, other seem to prefer yaml based ones. How can we know which one is suited for this use case?
A: This is not easy to answer.
If we expect that in the future every email will require its very own implementation of that thing which has been hook_mail for the last 13 years, then an attribute based plugin system is perfectly suitable.
If we expect that email in core takes a similar path as entities in core, and code sharing will get more and more common, then an attribute based plugin system is less useful. In the extreme case, we'd have to create empty plugin classes to put the attribute somewhere.
As pointed out by others (I guess it was @berdir), commerce already made a step in that direction. The commerce hook_mail implementation is almost empty.
Oh, wow! Completely failed reviewing a one line patch, sorry for that. Looks right now.
Thanks.
I propose to add 📌 Resolve potential namespace conflict with contrib mailer module Active to the list of issues which need to be resolved before 11.3.
Tagging with 11.3 release priority. This needs to be resolved before release.
I managed to reduce the main branch in 📌 Introduce email yaml plugin and an email building pattern known from controllers Active to around 600 LOC (without tests). I'm going to update the issue summary over there to better reflect the current state.
I'm trying to reduce the scope of the Draft PR in order to make it smaller and easier to review. As a first step, I'll be extracting features which aren't absolutely necessary to land in the first iteration. I'm thinking of the following things:
- Support for controller closure (already removed)
- Customizable
Emailclass (allows us to get rid of the custom plugin factory). - Move the builder code to another PR. While the builder simplifies the call-sites a lot, it doesn't need to be committed right away I think.
Anyone has additional suggestions?
I pushed a little refactoring of the renderer class. There is now an EmailControllerParams object passed through the whole process. This gets rid of the repeated ad-hoc $context definitions inside the render() function.
It also separates the langcode into an (alterable) environment parameters array. A hook_email_params implementation now can actually modify the language of the mail. The language switching itself is not yet implemented though.
In the MR. There is a requirements check for unknown / misspelled password algorithms.
When an algorithm is configured which isn't available on the system, attempting to create new password hash will result in a fatal:
Fatal error: Uncaught ValueError: password_hash(): Argument #2 ($algo) must be a valid password hashing algorithm in Command line code on line 1
Verifying existing hashes still works (assuming the hashing algorithm is available on the system).
Re #8 / #9, the 3539651-core-emails branch now contains a SystemEmailHooks implementation (an example on how this could work):
https://git.drupalcode.org/issue/drupal-3539651/-/compare/11.x...3539651...
By the time this ships, every single site which upgrades will generate a requirements warning. The requirements warning points to a CR which contains example code for a service provider class. The example code for the service provider class is there because I think we shouldn't instruct people to manually add a container parameter for this. The reason I think so is that it is safer to use the \PASSWORD_* constants instead of let people attempt to hard code the exact password algorithm into a yml file by hand.
Regarding the UI: This could be just a password_argon2 module with its own requirements check and a service provider which adds the password.algoritm: argon2id container parameter.
Pushed the EmailBuilder approach.
This code suggests that mail ID, parameters, and langcode will not be available at the point the email is being sent.
Good point. I think those should be added as tags / metadata to the Symfony Email object. This information is then also available on external mail service providers for filtering and routing.