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.
Thanks for the review.
Since this has come up multiple times now, I guess I'm going to iterate on the call site now. There are a couple of well known design patterns which might help in this situation. In my eyes, the current MailManagerInterface is a facade. It provides a convenience method (mail()) which abstracts away the whole complexity of mail plugins, formatting and logging.
Looking through core, I can find another well known design pattern which simplify access to complex subsystems. E.g., EntityStorageInterface::getQuery() returns an instance of a class implementing the builder pattern. The call site of entity queries resembles the example snippets by @zengenuity and @adamps. I think the builder pattern could be useful. E.g., a call site in the contact mail handler could look like this:
$key_prefix = $message->isPersonal() ? 'user' : 'page';
try {
$email = $this->emailManager->getEmailBuilder()
->id('contact.' . $key_prefix . '_mail')
->param('message', $message)
->langcode($recipientLangcode)
->build() # returns a Symfony Email
$this->mailer->send($email->to(...$recipients)->replyTo($message->getSenderMail()));
}
catch [...]
In cases where a message needs to be sent to multiple recipients with different languages, the builder can be reused (update module):
$builder = $this->emailManager->getEmailBuilder()
->id('update.status_notify')
->params($items);
foreach ($recipients as $recipient) {
[...]
try {
$email = $builder->langcode($langcode)->build();
$this->mailer->send($email->to($recipient));
$sent++;
}
catch [...]
In a second step, addressing, sending and exception logging could be moved into a convenience method. It doesn't need to cover all cases (build() is still there) but it could simplify the most used ones:
Contact example:
$key_prefix = $message->isPersonal() ? 'user' : 'page';
$result = $this->emailManager->getEmailBuilder()
->id('contact.' . $key_prefix . '_mail')
->param('message', $message)
->langcode($recipientLangcode)
->send(to: $recipients, replyTo: $message->getSenderMail(), logger: $this->logger);
Update example:
$builder = $this->emailManager->getEmailBuilder()
->id('update.status_notify')
->params($items);
foreach ($recipients as $recipient) {
[...]
if ($builder->langcode($langcode)->send(to: $recipient)) {
$sent++;
}
Noted the other comments in #6. All of them are useful as well, thanks!
This is a very weird issue. These are my observations so far:
Before
📌
Avoid reading session from the database multiple times during a request
Needs work
(git switch --detach 4e26ae9cc3da8b8a90561716c4a53fdd6c07f8f6):
- Session cookies are refreshed when big pipe module is enabled (e.g., standard install)
- Session cookies are not refreshed when big pipe module is not enabled (e.g., minimal install)
After
📌
Avoid reading session from the database multiple times during a request
Needs work
(git switch --detach 522404706e440106fe7e5d382ce018b0f89637d4):
- Session cookies are not refreshed when big pipe module is enabled (e.g., standard install)
- Session cookies are not refreshed when big pipe module is not enabled (e.g., minimal install)
At least this is now consistently working as expected by @cilefen.
I realized, that the OP reported something different:
In 10.2.x, the session cookie is updated if necessary on every request (not yet sure whether this is done by PHP, Symfony or Drupal).
After
📌
Avoid reading session from the database multiple times during a request
Needs work
, the call to \Symfony\Component\HttpFoundation\Session\Session::save() is too late to update the session cookie on big pipe responses. Response headers are already sent when big pipe performPostSendTasks() is called. I can reproduce this behavior by adding a cookie_lifetime: 30 container parameter (and still using session_write_interval: 20).
We could maybe fix that by adding more logic to \Drupal\Core\StackMiddleware\Session::handle(). E.g., trigger a session save unconditionally when the session timestamp is older than session_write_interval.
I tried to reproduce the issue with the following setup on 11.x:
- Standard install profile.
- Create a new user without admin privileges.
- Login with the new user.
- Repeatedly reload a page which triggers big pipe (e.g.
/contact).
In order to make the manual testing reproducible, I choose the following options for PHP session gc and Symfony session metadata_update_threshold (the latter is called session_write_interval in Drupal and it is a setting):
In sites/default/services.yml:
parameters:
session.storage.options:
gc_divisor: 1
gc_maxlifetime: 30
gc_probability: 1
In sites/default/settings.php:
$settings['session_write_interval'] = 20;
The effect of the PHP gc_divisor: 1 and gc_probability: 1 is that gc is run on every request before any session data is read from the database. That means that any session record with a timestamp older than 30 seconds is removed at the start of ever request (except for requests answered by the page cache of course).
The effect of the metadata threshold is that a session write is enforced for sessions with a timestamp older than 20 seconds.
With this setup I can confirm that every request which hits the window between the metadata threshold (session_write_interval and the gc max lifetime will bump the effective session lifetime by 30 seconds (gc_maxlifetime). This is expected behavior.
I can confirm that a session is removed and a user is logged out when I leave a tab open for more than 30 seconds (gc_maxlifetime) before I reload the browser (though, see the caveat at the end). That is expected behavior as well.
In my opinion this issue can be closed. I believe that the behavior observed by the OP is a result of a gc_maxlifetime value lower/equal than session_write_interval. If there is no window between the two values, the session timestamp is only updated when new session data is written. In cases where a very short session lifetime is required, it is probably best to set session_write_interval=0. With that setting, sessions will be written on every request - no matter whether the session data has changed or not.
For the latter test, I discovered an interesting edge case. For the test to pass there needs to be other activity which triggers the gc in between the requests in the tab left open. If the session record is still in the database when the request starts, then the session record is removed from the database (which is expected), but it is written at the end again and as a result survives the timeout (which is unexpected).
I'm unsure whether or not this edge case needs a fix. With the probabilistic gc in use today, sites cannot rely on session records being deleted reliably after surpassing the gc max lifetime anyway.
1) These lines seem to happen a lot. I feel it should be just one line to send a mail. The underlying layer can handle the separate build/render/send.
True. The call site needs more attention at some point. I'd like to keep the question open for a while to see what kind of abstraction best fits these cases. It very much depends also on whether or not we want to support passing a closure to $emailManager->getEmailDefinition().
2) I'm confused by inconsistencies between the modules, they seem far from following a pattern. Some have a builder some not. I feel that one class is enough and it can have separate functions for the parts. All modules can have the same 2 functions.
I tried to present many different approaches in 3539651-core-emails. E.g., the confusingly named ActionMailBuilder class is an action plugin and a mail builder at the same time (demoing the closure approach). The UpdateMailBuilder shows that it is still possible to create text-only emails (question is whether we want to support that or not). UserNotificationHandler shows the closure approach with first class callable syntax, ContactMailHandler shows how it could look like if the "controller" is set via yml file.
If we want to continue with this approach for a bit longer, we should probably decide which pattern is to become the standard one.
The $langcode is tricky and indeed merits an in-depth discussion. I think it is worthwhile to consider it thoroughly whether it should remain a required parameter (like it is today, and like it was on all my prototypes) or whether it should be dropped (as suggested by some comments). And if we decide to drop $langcode from the call-site method signature, we should discuss that further with language subsystem maintainers I guess.
From the top of my head, I can think of at least four ways to determine the language for an outgoing mail:
- Use the site default language.
- Use the negotiated language (this only works if the mail sending is triggered in response to a browser request, it falls back to 1 on cron runs).
- Use the recipients preferred language (if the message has only one recipient and it is linked to a known user account).
- Use a language stored on a related entity (e.g., the language could be stored on a newsletter subscription).
It seems to me that the choice of language is context dependent. This could be an indicator that indeed the call-site is the most appropriate location to set the langcode. At the call-site, the negotiated language is still available (via ConfigurableLanguageManager::getCurrentLanguage()). Depending on how the context switching is implemented, the negotiated language might return a different result inside the mail builder (i.e., inside the switched context). That in turn means, that mail builders have less options to choose from when attempting to determine the correct message language.
The call-site also might be aware of whether or not it is running in cron (update) or from within a form submission. Issues like 🐛 Password reset invalid mail notify language Needs work also indicate, that some users expectations are different than what others find intuitive.
Keeping the $langcode on the call-site method signature forces developers to make a sensible choice. It is for sure debatable whether or not this is a good thing.
On the other hand, neither the old interface nor any of my prototypes has references to other i18n information. E.g., for certain messages it would be important to know the recipients timezone (e.g., calendar invites). So why do we have $langcode as a required parameter on the call-site method signature but not $timezone?
Added the requirement checks.
Should we add a config and a proper UI to make these requirement checks more actionable? Maybe an additional Password fieldset in admin/config/people/accounts?
znerol → created an issue.
znerol → created an issue.
Any call to the old function continues to work as-is but doesn't go through the new API. It would mean they wouldn't use the new mail API once replaced, but that's perfectly fine IMHO, especially as long as the whole thing is still experimental.
Exactly. This is the responsibility of NotificationHandler::sendCompat(). A replacement service implemented by the mailer module either could override it (and throw an exception to make it explicit that _user_mail_notify() isn't supported by the experimental mailer). Or it could leave it alone and then a mail would be created and sent by the legacy mail system.
The individual functions are fine. They provide a good place for docs and we get rid of the $op strings at the call site.
I updated the issue summary and the change record. This is ready from my side.
I went through a gitlab code search (
#3051266: Searching for code across all of Gitlab →
) in order find out how _user_mail_notify is used in contrib. In order to exclude core clones and distros, I used the following query:
_user_mail_notify -path:core/modules/user -path:modules/contrib
That returns 228 results. I went through half of them and manually assessed how the function is being used. There are two bigger groups and a couple of interesting special cases.
Modules dealing with user registration
- https://www.drupal.org/project/better_register →
- https://www.drupal.org/project/bulk_user_registration →
- https://www.drupal.org/project/business_core →
- https://www.drupal.org/project/cas →
- https://www.drupal.org/project/civicrm_entity →
- https://www.drupal.org/project/cognito →
- https://www.drupal.org/project/commerce →
- https://www.drupal.org/project/commerce_guest_registration →
- https://www.drupal.org/project/created_account_register_message →
- https://www.drupal.org/project/crowd →
- https://www.drupal.org/project/entrasync →
- https://www.drupal.org/project/firebase_authentication →
- https://www.drupal.org/project/iyo →
- https://www.drupal.org/project/jsonapi_user_resources →
- https://www.drupal.org/project/minimal_register →
- https://www.drupal.org/project/nodehive_core →
- https://www.drupal.org/project/openlucius →
- https://www.drupal.org/project/opigno_learning_path →
- https://www.drupal.org/project/resend_register_mail →
- https://www.drupal.org/project/resend_user_login_link_user_operation →
- https://www.drupal.org/project/social_auth_hid →
- https://www.drupal.org/project/user_csv_import →
- https://www.drupal.org/project/webauthn →
- https://www.drupal.org/project/webform_user_registration →
Modules dealing with password resets
- https://www.drupal.org/project/admin_password_reset →
- https://www.drupal.org/project/diba_security_policy →
- https://www.drupal.org/project/eep →
- https://www.drupal.org/project/login_emailusername →
- https://www.drupal.org/project/login_flow →
- https://www.drupal.org/project/mass_password_change →
- https://www.drupal.org/project/mass_pwreset →
- https://www.drupal.org/project/openstory →
- https://www.drupal.org/project/pass_reset_checkbox →
- https://www.drupal.org/project/passwordless →
- https://www.drupal.org/project/patreon →
- https://www.drupal.org/project/rest_password →
- https://www.drupal.org/project/rest_register_verify_email →
- https://www.drupal.org/project/secure_password_reset →
- https://www.drupal.org/project/social →
General purpose / all ops
- https://www.drupal.org/project/if_then_else →
- https://www.drupal.org/project/mail_debugger →
- https://www.drupal.org/project/rules →
- https://www.drupal.org/project/user_account_emails →
- https://www.drupal.org/project/user_api →
Special cases
GSIS (GOV.GR) Login →
They are actually in the first group (Modules dealing with user registration) but they do just call $account->activate(); to trigger the registration email.
Own implementation
The following modules ship their own version of _user_pass_notify.
- https://www.drupal.org/project/group_notifications →
- https://www.drupal.org/project/magic_code →
- https://www.drupal.org/project/user_registrationpassword →
Custom op
The Forgot username → module actually did implement a custom op (sight!).
I think we cannot deprecate _user_mail_notify as a whole without providing proper APIs to cover most of these use cases first. That said, I think the best way forward would be to deprecate custom ops here in a first step.
In the new mail system, Contrib would like to be able to trigger generation of emails. I believe Easy Email might include a UI for this feature generally. Commerce has it specifically as "resend receipt".
Yes, of course.
% ./vendor/bin/drush updb
-------- ------------------ ------------- ------------------------------------
Module Update ID Type Description
-------- ------------------ ------------- ------------------------------------
system password_kernel_ post-update Rebuild the container after adding
parameters new kernel parameters.
-------- ------------------ ------------- ------------------------------------
┌ Do you wish to run the specified pending updates? ───────────┐
│ Yes │
└──────────────────────────────────────────────────────────────┘
> [notice] Update started: system_post_update_password_kernel_parameters
> [notice] Update completed: system_post_update_password_kernel_parameters
[success] Finished performing updates.
CR is ready. Also pushed an update to the StackedKernelPass docs.
This is literally a beer idea, curtsy of @jurgenhaas → . I'm recording this here just for completeness:
If we think
📌
Introduce email yaml plugin and an email building pattern known from controllers
Active
one step further, email could just be one of the supported response formats. In order to render an email, create a Request object with appropriate attributes, dispatch it through the http kernel as a subrequest, grab the email object from the response and send it via the transport framework to wherever its needed.
On the way through the request / response cycle, parameter upcasting, language negotiation, theme negotiation, render context isolation, render caching and maybe even cooperative multitasking via fibers is performed just like with every other request. Without experimentation its impossible to know what works and what breaks, though.
It would also be interesting to explore whether or not this code flow would simplify access checking on embedded attachments and images.
Considering email builders as routes (or microservices, if you prefer) also raises interesting questions. For example, some email builders likely are idempotent (simple notifications are always the same when generated with the same input) while others are not (the password reset link must be newly generated).
MaintenanceModeSubscriber uses BareHtmlPageRenderer nowadays.
I started working on a new approach for the core mail building infrastructure: 📌 Introduce email yaml plugin and an email building pattern known from controllers Active
I think this should feel quite familiar for people who are used to implement their own controllers.
In this iteration I also put more effort in the HTML/templating part. I took a look at the entity templates (node.html.twig, taxonomy-term.html.twig, media.html.twig, etc.). All of them use a contents variable containing a list of content items. I think it would be good to use the same pattern in email.html.twig.
I moved the language switching to the language module. On monolingual sites this is now a no-op.
Also found a use case for this in core: HelpTopicSection::renderTopicForSearch().
Updated the deprecation message and the change record. _user_mail_notify() is replaced by an internal service. It follows, that there is no replacement API, this must be reflected in the CR.
I restored the BC for _user_email_notify() and also the return type behavior. Dispatching op to method call in _user_mail_notify() is actually more work than keeping BC.
I also found a way to remove the interface. It is possible to mark methods for setter injection with the #[Required]. That way, a subclass doesn't need receive and pass on a MailManager via its constructor. It only needs to declare the services which it really needs.
I tried to update the issue summary in order to be more precise about the intention here. Regarding #20, this issue is about a different abstraction layer. It has no influence on the design of a future mail building layer.
Regarding the discussion whether this should be enums or separate methods:
🐛
Use email verification when changing user email addresses
Needs work
would definitely profit from separate methods. The new email address could be passed in via an additional argument. The current MR has to tunnel it via the param array. This could be solved much better with a method with dedicated signature: https://git.drupalcode.org/project/drupal/-/merge_requests/5773/diffs#di...
Moving this under the umbrella of 🌱 [META] Adopt the symfony mailer component Active .
I removed the BC for custom $op parameters. Instead _user_mail_notify now maps the $op to the correct method.
One thing to note is that this also changes the return value. Before, FALSE was returned on error and NULL when mail sending was suppressed (either by config or by a hook). The new methods return FALSE in both cases.
Rebased for OOP hook conversion.
From the IS:
Don't re-use user_mail_notify or mutate the email on a cloned version of the user in case someone calls ::save() on it
I think this would be straight forward to implement after 📌 Extract _user_mail_notify() into a user MailHandler Active .
Issue 🐛 Use email verification when changing user email addresses Needs work seeks to add two new operations. It would be good to land this and the other issue in the same release.
Interesting observations in
🐛
_user_mail_notify return error when email sending is aborted
Active
regarding the _user_mail_notify() return value.
📌
Improve documentation of _user_mail_notify
Postponed
regrettably ended up in a bike-shed discussion on whether it is necessary to update coding standards before committing the docs improvement. The MR in
✨
Use an enum for user email notification types
Needs work
tries to change every single occurrence of an $op like string regardless of the context. By implementing the proposal from #2 (individual methods on a MailHandlerInterface), it is possible to circumvent both of these pitfalls.
The MR here turns the $op argument into an internal implementation detail. It isn't API anymore and therefore may remain a string.
#3463923-11: Improve documentation of _user_mail_notify → points out an interesting angle:
I must say I don't particularly like this, the enum would be much nicer. Regardless, I think it's possible (even if ill-advised) to alter or extend the available options, so this might be needlessly restrictive.
In Drupal 7 it was indeed very easy to extend the list of possible operations via a contrib or custom code. Simply by adding a pair of variable_set() calls in an .install file ensuring that user_mail_OP_subject and user_mail_OP_body contain some meaningful text. I haven't tried it, but it still might be possible to do something like this with a smart combination of form alters and config schema hacks.
This practice should therefore be deprecated properly.
We can revisit rescope this later.
This hasn't sparked any interest and is not being worked on at the moment.
Approach abandoned in 📌 [meeting] 2025-07-29 core mailer beta planning Active .
Approach abandoned in 📌 [meeting] 2025-07-29 core mailer beta planning Active .
FWIW I would put this issue in the "Mail building Components" section in the roadmap.
I'm okay with that, just do it.
I added that to 🌱 Mailer module roadmap: the path to beta and stable Active . It seems straight forward and actionable.
I found
#3229038: Consolidate public APIs for generating user action URLs into a service →
but that doesn't seem to touch _user_mail_notify() therefore filed
📌
Extract _user_mail_notify() into a user MailHandler
Active
.
We decided against the config + plugin approach in our last meeting. Due to the risks of config going missing and due to the fact that recipes do not encourage installing default config.
An issue with the current architecture is that there's a bidirectional dependency between the plugin and the config entity, as it's passed to render. If you compare with block plugins for example, they don't know about block.module block config entities and can be used in other contexts as well. Similar for field formatters, which are also used by views. That's what $configuration is for, the config entity is responsible for storing and passing that in.
That is crucial and it has bothered me a lot. I think I'll try to decouple the config from the plugin and then use that to branch off for coming experiments.
From #3534136-21: Requirements and strategy for Core Mailer → by @berdir:
Catching up on the discussion and merge requests. Long comment incoming...
I don't think LOC is the primary factor to consider for this. Also because the current examples are way more complicated than they need to be due to feature flags, the experimental module and BC. The actual implementation would be a lot less.
That said, I do agree that the config entity approach seems complex. Yes, config entity + plugin are a very common pattern, but sometimes also overused. This feels similar to action plugins and config entities, which are often pretty awkward as it's typically a 1:1 relationship and tedious for modules to maintain the config. In general, code that is hardcoding specific config entity ID's is a bit of a code smell to me. As @adamps said, it's unclear what would happen if that config is missing. For example now with recipes, which pretty much *encourages* installing modules without their config ( I don't really agree with that, but that's another topic). Deleting the password reset template
Naming: I think the template part is misleading. That makes me think of twig templates, HTML and customizing their output. Looking at DSM, it uses build() instead of render(), which makes a lot more sense to me. We build an Email object, which might or might not involve rendering something or using templates, but it's not the main thing. It's also similar to terminology used in simplenews.
An issue with the current architecture is that there's a bidirectional dependency between the plugin and the config entity, as it's passed to render. If you compare with block plugins for example, they don't know about block.module block config entities and can be used in other contexts as well. Similar for field formatters, which are also used by views. That's what $configuration is for, the config entity is responsible for storing and passing that in.
Now, there are cases where a fully implemented config entity + plugin could be useful. For example with user, the plugin would be provide a subject and body setting which would make it self-contained instead of accessing global config (which currently bypasses your attempt at making not just mails but also their data and arguments documented and discoverable). and status is built-in, so the code could check that. But it would mean that the UI would move from user settings to generic mail configuration, which is likely harder to find. It's also not as flexible as you'd be used to from plugins. you can't make up your own notifications, you can only enable/disable and edit the specific settings of them. In the end, pretty limited benefit over the current config and some drawbacks.
contact, update are examples that don't really benefit from there being a config entity, and that's going to be a lot of contrib and custom examples. That's combined with the "problem" that the mail rendering/preparing sits entirely on top of the actual mail send API and is there fore optional. People looking at this and trying to send mails will just directly create the Email object and send it directly. And if they do that, we lose the ability to use a mail theme because rendering of stuff possibly already happened. I think we should make it as easy as possible to use the mail API without losing features like language. And then optional features on top. If you look at commerce_mail(), they essentially bypass the whole thing by just passing in the body and rendering it there. I wonder if we should actually allow to use a callback based approach for those minimal cases, similar to what renderInContext() essentially already is.
I'm not sure how important discoverability and reusability of mail "templates" really is, it might in some cases even be undesired. We recently discussed that ECA explicitly doesn't support those special user password reset mails, to not allow to send them out in the wrong context, that could even be a security issue.
On the other side of the spectrum, use cases that actually have generic and complex use cases around mails might struggle to fit into this architecture. Simplenews has multiple things that make up a newsletter, specifically a entity that sent to a subscriber and there's a lot of rendering and logic going on that should happen within the mail render context. And ECA would need to redefine its mails that sit in its configuration or have a super generic one that would almost become recursive.
I think I get the intent with the types and data, but that really feels complex to use. the user confirm thing requires a data type plugin, a definition class, it's passed in as a raw value, converted to typed data and then used. It values (theoretical) reusability over DX, despite all that complexity, what you work with when writing the code is an array with arbitrary keys. I see that DSM has the concept of mail params on the object. That's still keys and not types that can be enforced, but I think that's an improvement over the single magic mixed type that this has. With mulitple params, we can still document them and their type.
I've even been thinking about going entirely with plain value objects implementing a basic interface, but haven't fully thought that through. But overall I'd clearly recommend keeping the actual mail builder interface/API as plain and un-opinionated as possible and start with that.
I think the dates in #12 should be 2015 rather than 2026?
Thanks, fixed.
Would a service locator be an option here instead of the lazy services pass? We've been switching to those in 📌 Replace lazy service proxy generation with service closures Active : Remove lazy declaration and proxy class for cron and use service closure instead and similar issues.
I haven't looked into service locators. The cited issue is using service closures (same tech as the PR). Lazy services are not part of the fix (the dysfunctional lazy flagging is removed in this PR).
As for the question whether it would be possible to do this without a custom service pass: I do not yet know. The service pass has two responsibilities: Assemble every service tagged with http_middleware into a stack. This is done by prepending a reference to the decorated service to the arguments array of the decorator. The second responsibility is to inject all discovered middlewares into the StackedHttpKernel. This is required to propagate the terminate call. Maybe the symfony container provides all the necessary tooling to realize that. But even then, it would be necessary to add some kind of BC pass to move old declarations (tagged with http_middleware to their spot in the stack. I'd be interested to explore. Not sure whether this should be done in this issue or in a follow-up.
The failure of UncaughtExceptionTest::testLoggerException() is interesting. This test examines the PHP error.log in order to check whether exceptions get logged when the normal logger failed.
https://git.drupalcode.org/issue/drupal-3538294/-/pipelines/560646/test_...
Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest::testLoggerException
The error + the error that the logging service is broken has been written to the error log.
Failed asserting that actual size 12 matches expected size 10.
core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php:251
I reproduced it locally and found that NegotiationMiddleware appears in the stack trace with the patch, but doesn't on the 11.x branch. The reason is that MonkeysInTheControlRoom middleware is registered with the same priority. But since PriorityTaggedServiceTrait::findAndSortTaggedServices() uses a different method of sorting, the middlewares swapped their location in the stack.
Bumped the monkey middleware to an appropriate priority to solve the test fail (404).
(forgot to attach raw data)
Drupal CMS baseline:
% composer create-project --no-interaction "drupal/cms:^1.2.x-dev" cms-dev
# Enable some recipes. I had to disable case management and project management due to field storage issues..
# Disable css/js aggregation (not compatible with php built-in webbrowser)
# Rebuild container, clear caches, warm cache
% ab -c 1 -n 500 http://localhost:8888/ > ./cms-dev-baseline.txt
Requests per second: 29.76 [#/sec] (mean)
Time per request: 33.597 [ms] (mean)
Time per request: 33.597 [ms] (mean, across all concurrent requests)
Drupal CMS + patch:
% patch -p1 < /tmp/perf.patch
% # Rebuild container, clear caches, warm cache
% ab -c 1 -n 500 http://localhost:8888/ > ./cms-dev-patched.txt
Requests per second: 44.32 [#/sec] (mean)
Time per request: 22.563 [ms] (mean)
Time per request: 22.563 [ms] (mean, across all concurrent requests)
Performance (Drupal core standard install)
Software:
% uname -v
#1 SMP PREEMPT_DYNAMIC Debian 6.1.140-1 (2025-05-22)
% php -v
PHP 8.3.23 (cli) (built: Jul 3 2025 16:20:45) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.3.23, Copyright (c) Zend Technologies
with Zend OPcache v8.3.23, Copyright (c), by Zend Technologies
with Xdebug v3.4.4, Copyright (c) 2002-2025, by Derick Rethans
% podman ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
2967cbf5773c docker.io/library/postgres:16 postgres 4 minutes ago Up 4 minutes ago drupal_memory
Webserver:
% export PHP_CLI_SERVER_WORKERS=4
% php -S localhost:8888
Core baseline:
% composer create-project --no-interaction "drupal/recommended-project:^11.x-dev" core-dev
# Install core standard profile
# Disable css/js aggregation (not compatible with php built-in webbrowser)
# Add node/1
# Rebuild container, clear caches, warm cache
% ab -c 1 -n 500 http://localhost:8888/node/1 > ./core-dev-baseline.txt
Requests per second: 44.61 [#/sec] (mean)
Time per request: 22.415 [ms] (mean)
Time per request: 22.415 [ms] (mean, across all concurrent requests)
Core + patch:
% patch -p1 < /tmp/perf.patch
% # Rebuild container, clear caches, warm cache
% ab -c 1 -n 500 http://localhost:8888/node/1 > ./core-dev-patched.txt
Requests per second: 55.61 [#/sec] (mean)
Time per request: 17.984 [ms] (mean)
Time per request: 17.984 [ms] (mean, across all concurrent requests)
The CR is a stub. Help welcome.
#2408371: Proxies of module interfaces don't work →
introduced ProxyServicesPass. But that runs before StackedKernelPass. So the lazy flag added StackedKernelPass to middleware definitions on top of the first responder has no effect.
I tried to move the StackedKernelPass before ProxyServicesPass.
diff --git a/core/lib/Drupal/Core/CoreServiceProvider.php b/core/lib/Drupal/Core/CoreServiceProvider.php
index cd9766c14b5..26cab2964e6 100644
--- a/core/lib/Drupal/Core/CoreServiceProvider.php
+++ b/core/lib/Drupal/Core/CoreServiceProvider.php
@@ -73,14 +73,14 @@ public function register(ContainerBuilder $container) {
$container->addCompilerPass(new SuperUserAccessPolicyPass());
+ $container->addCompilerPass(new StackedKernelPass());
+
$container->addCompilerPass(new ProxyServicesPass());
$container->addCompilerPass(new BackendCompilerPass());
$container->addCompilerPass(new CorsCompilerPass());
- $container->addCompilerPass(new StackedKernelPass());
-
$container->addCompilerPass(new StackedSessionHandlerPass());
$container->addCompilerPass(new MainContentRenderersPass());
But ProxyServicesPass doesn't like that:
[warning] Missing proxy class 'Symfony\Component\ProxyClass\HttpKernel\HttpKernel' for lazy service 'http_kernel.basic'.
Use the following command to generate the proxy class:
php core/scripts/generate-proxy-class.php 'Symfony\Component\HttpKernel\HttpKernel' "[namespace_root_path]"
ProxyServicesPass.php:63
Besides the HttpKernel, the following middleware classes are reported:
Drupal\Core\ProxyClass\StackMiddleware\ContentLengthDrupal\Core\ProxyClass\StackMiddleware\KernelPreHandleDrupal\Core\ProxyClass\StackMiddleware\SessionDrupal\big_pipe\ProxyClass\StackMiddleware\ContentLength
There are for sure more missing in contrib and custom code. Thus, trying to fix the lazyness is not an option I guess.
Went through an epic git bisect only to find that this broke just two months after it was introduced. Everything happened before 8.0 was even released.
Lazy middlewares and responder tag introduced in: #2473113-47: All stack middlewares are constructed at the same time even for cached pages → commit date: 2025-04-30
Lazy middlewares and responder tag broken by: #2408371-83: Proxies of module interfaces don't work → commit date: 2025-06-30
Thank a lot @berdir for the review.
And if it doesn't, should the code setting that up be removed?
Yes. In a follow-up.
I do not know what happened to lazy services since #2473113: All stack middlewares are constructed at the same time even for cached pages → . They are being removed gradually anyway, so didn't bother to look.
Setting to NR for early feedback. This still needs tests for the deprecation in the page cache middleware. I think we should deprecate the responder tag attribute as well. This could be done in a follow-up though.
Added a draft MR. This prevents the event dispatcher from being instantiated when a page is delivered from the cache.
Not on cached pages. The performance regression exists on every page if you have enough listeners.
Then please submit your PR as a separate issue.
a performance regression is present since 10.3 for any site with a large number of listeners regardless of cached pages.
The performance regression was reported for 11.1 and is clearly measurable on 11.1 on cached responses. Is there any evidence that 10.3 suffered a performance regression on cached pages of the same scale?
I guess this could be solved with a service closure for the $http_kernel argument in PageCache middleware.
Event dispatcher shouldn't be instantiated for cached pages at all. Filed: 🐛 Regression: All stack middlewares are constructed at the same time even for cached pages Active .
znerol → created an issue.
Thinking the approach from #37 a bit further. This adds a requirements check to better illustrate the situation.
Some people still want to disable the Password Compatibility module (after switching to Argon2id). Thus, I guess that doesn't need to be moved back to core.
Update the steps to reproduce and did manual testing for the fix.
There are two things which are standing in the way for an automated test here:
- The repro requires the SMTP transport (because that is the only built-in transport which attempts to log anything during service destruction). The test systems tries very hard to make sure that no email is sent during automated tests.
- The error occurs extremely late in the request-response cycle. It errors when the response is already sent to the simpletest parent site, so we cannot use response headers to communicate it. And we cannot use the database to communicate back the result, because the database connection has been closed already as well. The only thing which could work would be to attempt to search the webserevr / php error log.
Attempting to write an end-to-end test for this case would most probably result in another complex, hard to debug and potentially random failing edge case test.
We are dealing with an experimental plugin here. I propose to either fix it with the proposed one liner or just close the issue as a won't fix.
Thanks! You could remove the public static create method and adapt the constructor as well. This class is explicitly experimental and marked @internal. No need to maintain backwards compatibility here.
znerol → changed the visibility of the branch 3420372-core-symfony-mailer to hidden.
Added a new MR which builds on top of the length limit approach: When creating a new password hash which is unsuitable for bcrypt, use phpass as a fallback.
That could be accompanied by a requirement check: If bcrypt is in use, then recommend to either enable phpass or switch to argon2.
Nextcloud is using argon2 since a long time. It appears that they introduced it around NC 14.0.2 (2018).
There are reports on nextcloud forums about login problems after server migrations though. But it looks like people are able to figure out what is causing them.
Argon2 support in Fedora 40 is available via libsodium:
# dnf install php-cli php-sodium
Argon2 support in CentOS 9 is available via epel and libsodium:
# dnf install epel-release
# dnf install php-cli php-sodium
It seems to me that argon2 support is widely available in PHP distro packages. I haven't found any distro which is shipping PHP without argon2 support lately.
Notable exceptions: Fedora 40 (and older), CentOS 9 (and older) https://github.com/znerol-scratch/php-argon2-survey
It seems to me that argon2 support is widely available in PHP distro packages. I haven't found any distro which isn't shipping PHP without argon2 support lately.
I guess the reason why PHP did not switch to argon2 for password hashing by default is because they still are not sure which implementation they want to use. They have compile time support for argon2 via libargon2, via libsodium and recently they added argon2 via openssl (PHP 8.4).