- πΊπΈUnited States Chris Matthews
For clarify, does this issue need to be moved to the "Ideas" issue queue so that the decision to officially "adopt" this "child" can be made? If not, should this issue be a 'feature request' or a 'plan'? Seems like in order for it to be a 'plan' it must first become the community agreed upon path forward.
- π¨πSwitzerland znerol
For clarify, does this issue need to be moved to the "Ideas" issue queue so that the decision to officially "adopt" this "child" can be made?
Several people expressed their support for #31 (e.g., @alexpott in #41 and ff). Also #51 already argued against turning this into an idea. Plan is the correct category for this umbrella issue.
The logical first step is to get #3165762 in. That issue has a small enough scope to actually have a chance to succeed.
- π¬π§United Kingdom adamps
Drupal Symfony Mailer β module now has a stable release with 8k sites using it. Compatibility with swiftmailer is quite good. The "ticking time-bomb" has substantially been prevented. In the end we chose a different approach and β¨ Create a Mail plug-in that emulates swiftmailer Closed: outdated was not implemented.
This has given a reasonable insight into how to integrate Symfony Mailer into Drupal. However of course that module is complex and so not in itself a logical first step to be added to Core.
The logical first step is to get π Add symfony/mailer into core RTBC in. That issue has a small enough scope to actually have a chance to succeed.
I agree that we need to make a first step. However It feels like we're trying to take a first step without agreeing where we're headingπ. This issue has category = plan. I suggest that the first thing we need to do here is write a plan with a list of goals, and a sequence of steps to achieve them.
It feels that some work is needed to show that #3165762 is the right first step:
- State a clear purpose (the IS is mostly empty).
- Work out how we would later add new function. Currently the sending code cannot access the Email object, there is no way to configure the transport, there is no handling of HTML emails.
- Check compatibility. The current patch locks all sites using CoreRecommended to a specific version of
symfony/mailer
which might cause a conflict with other contrib modules or break existing sites. - Allow for further development. We don't want to promise BC which might make it difficult to add new function. This could be resolved by marking it as experimental.
- Review details. The code has likely been simplified rather too much - in comparison swiftmailer is much bigger. E.g. splitting a list of emails by php
explode
fails if the display name contains a comma.
- π¨πSwitzerland znerol
I got the chance to moderate a core conversation session recently at Drupal Dev Days in Vienna (recording, slides).
The discussion touched the following topics:
Why introduce the symfony mailer component in the first place?
- Important mail transports out-of-the box (SMTP, etc.).
- Maintained in symfony repository.
What is the procedure of introducing new dependencies?
New dependencies can be introduced in minor versions. Breaking changes (remove old APIs) are only possible in a major release.
How to suppress delivery during development or force redirection in test environments?
- The NullTransport can be used to disable delivery (DSN:
null://null
). - Headers can be modified in a subscriber to the MessageEvent.
How to provide backwards compatibility for
hook_mail_alter
?This was probably the most complex topic discussed. It became apparent, that converting message arrays (as required by
hook_mail_alter
) to Email objects and vice versa would be quite a challenge. Mainly because message arrays are extended by contrib and custom code with keys and values core doesn't have any knowledge about. During the discussion, two approaches to this problem were examined:- Core starts to restrict the set of key-value pairs which may be present in the message array by deprecating certain combinations. In order to make that work, it would be necessary to provide new APIs for those use cases before.
- Core introduces the
symfony/mailer
component, but without switching right away its own mails to the new API. From that point in time contrib modules implementinghook_mail_alter
should be extended to additionally provide aMessageEvent
subscriber mirroring the logic in theirhook_mail_alter()
implementations. After contrib and custom modules had some time to adapt, core will switch its own transactional mail to the new API and deprecateshook_mail
andhook_mail_alter
.
Multiple participants favored the second options.
How to pass context to
MessageEvent
subscribers?Problem: Implementations of
hook_mail_alter
can access theid
,module
andkey
properties in order to check where a mail came from. But there doesn't seem to be a way to pass context via theMessageEvent
.- Symfony seems to resolve that with custom headers on the mail message (e.g.
X-Transport
for transport selection). - Another way to make messages distinctive would be to leverage the PHP type system. Modules would subclass the
Email
class for each transactional mail. However, requiring subclasses which do not implement any logic would be a bit pointless.
How a message is built in the absence of
hook_mail
?Pass an
Email
subclass which accepts a render array into the mailer. The symfony mailer MessageListener then calls a Drupal specific implementation of the BodyRendererInterface which turns the render array into markup.This way the full power of the Drupal theming system can be leveraged to build, extend and alter the body of mails.
Could this be built in contrib and ported into core once its finished?
Contrib already provides a big selection of mail related plugins. Getting a contrib module into core has proven to be a big effort in the past. We have the tools in place to iteratively develop things in core in manageable steps.
Would core need to provide support for multiple transports
I.e., is there a use case for mails from one module sent via SMTP, all other mails sent via another transport)?
Very likely core doesn't need to cater for that case.
- π¨πSwitzerland berdir Switzerland
> How to pass context to MessageEvent subscribers?
I also proposed to have a standard subclass that essentially keeps the concept of module/key identification by enforcing it through a create/factory method. A service factory to get an e-mail object could also allow contrib/custom to replace it with a different class with additional functionality if there are use cases for that.
> Would core need to provide support for multiple transports
As discussed, actually different real transports seem like a very unusual use case. There are some interesting cases on top of that, I saw that there's an async option, where the actual sending (and rendering) is delayed, that could be interesting for simplenews and other bulk e-mail use cases. As well as rerouting and so on. But more than happy to let contrib explore on this, we can always add more features to core later on if they are widely useful.
> How to provide backwards compatibility for hook_mail_alter?
I still have plenty of questions about the second option, but we can hopefully figure that out as we go. The next steps are clear, defining and getting a new API into core and using it at least for tests that don't need to provide BC. One idea on top of the second option that I just had that could be very interesting is to introduce a setting. We've done this in the past for behavior changes. Basically you'd opt in to core using the new mailer API or not. Contrib is free to respect that or not, though they are of course welcome to do so as it would help sites transition to the new API. The setting could at first be opt-in and experimental and at some point we could switch it to on by default for new installs, deprecate the old setting and eventually remove the setting together with the old API.
Sure, would be a bit of work to maintain both implementations at once, but core only has a handful of mail implementations, that seems quite manageable.
- π¬π§United Kingdom adamps
> How to provide backwards compatibility for hook_mail_alter?
In Drupal Symfony Mailer, we do option 2. We provide a controlled migration process from the old system to the new, allowing some modules on old and some on new. The old system calls hook_mail_alter(), the new system calls the new hooks. There is no hook BC - that seems hard/impossible.> How to pass context to MessageEvent subscribers?
In Drupal Symfony Mailer, we have extended the Email class, adding$type
and$subType
equivalent to module and key in the old API.However we don't use MessageEvent instead we created something else.
Drupal\symfony_mailer\Email
extends\Symfony\Component\Mime\Email
using the decorater pattern - it has aprotected $inner;
of type\Symfony\Component\Mime\Email
. This was done to allow changes to make things more Drupal like. For example the original class hasfunction subject(string $subject)
which forces translation of any string potentially before the correct language has been set. Also the original class limits extension with private members, Drupal names set-accessorssetSubject()
rather thansubject()
, it allowed us to create an interface, etc.> How a message is built in the absence of hook_mail?
In Drupal Symfony Mailer, we have@EmailBuilder
plug-ins.> Could this be built in contrib and ported into core once its finished?
It already exists in contrib in Drupal Symfony Mailerπ. We can use that as a reference as we build up function in Core step-by-step. - π¬π§United Kingdom adamps
The migration path from old system to new is quite complex. We need to migrate at least 4 things:
- email builders:
hook_mail()
- email altering:
hook_mail_alter()
- email config/forms: user.mail.yml etc: allow HTML body and all the other possibilities of a modern email interface
- email transports: implementations of
Drupal\Core\Mail\MailInterface
The only solution I can see is a phased transition, where the two mail systems exist in parallel in the code. First the new system is experimental, then it becomes stable, then the old is deprecated, then the old is removed.
Core modules would provide both old and new mail systems during the transition. Contrib modules could do the same, or switch from old to new with a major version change.
In Drupal Symfony Mailer we have
MailManagerReplacement
which implement the old API and converts to the new (calling the old hooks). This allows removal of the old MailManager and old transports whilst continuing to support modules using the old building/altering code.We can have a config setting for which mailsystem to use overall, and another for each module (for those that provide both old and new email building). Site builders can first switch mailsystem/transports, get that working then switch modules one-by-one or all at once depending on the stability of each one in each environment.
- email builders:
- π¬π§United Kingdom joachim
This issue is starting to veer into π Add an alternative to hook_mail() and deprecate it Needs work , where I posted a detailed plan for deprecating hook_mail().
- π¬π§United Kingdom adamps
Key design summary β of Drupal Symfony Mailer.
Naturally core will want to simplify, but still most of these classes exist for a really good reasonπ. As a minimum I would expect to need Mailer, Email, EmailBuilder.
- π¬π§United Kingdom adamps
This issue is starting to veer into #1346036: Add an alternative to hook_mail() and deprecate it, where I posted a detailed plan for deprecating hook_mail().
It's true, both issues will add an alternative to hook_mail() and deprecate it. I looked at your module as part of writing Drupal Symfony Mailer, and I like the ideas there.
The key is that this issue has some huge advantages: we totally get rid of our own mail system in favour of a high-quality, well-supported, third party one which has loads of useful features especially HTML mail and a wide range of transports out-of-the-box. For all this, people are willing to undertake the effort of migrating the entire mail ecosystem. The community seems to have enthusiasm to complete this issue.
To make it work, we need to closely match what symfony does.
- Email is a class to match symfony rather than being a config entity (also a class is good performance for modules like simplenews that can send 50k emails in one go). This automatically gives us body, subject, address and many things.
- However, we can extend the symfony Email class to add tokens, languages and many other Drupal things. Drupal Symfony Mailer has method to render a content entity, which picks up the idea you had of mail_message entities.
- Email transports (back ends) are already defined by symfony. We can extend them with a config GUI (maybe in contrib?)
I'm maintainer of Drupal Symfony Mailer, and it's a good template for what can happen here. I'm sharing what I've learnt. I expect that some of it will be taken into core, some not. You are maintainer of Mail. It's less close to what's happening here, but still many of the ideas can be adapted, so you can share ideas too.
- π¨πSwitzerland berdir Switzerland
> The migration path from old system to new is quite complex. We need to migrate at least 4 things:
IMHO the main challenge is hook_mail_alter() and the behaviour of the system as a whole which is more or less the combination of all 3-4 points customized to a specific site. The hook_mail() implementations themself can change on its own whenever they want to convert to the new system. But hook_mail_alter() is unpredictable, you will have custom and contrib implementations, for example we alter user mails to a set a format so they can be sent as HTML with swiftmailer. Once user module switches to the new API, that just stops working.
3. is IMHO a separate system on top, see also below.
4. Those modules will then get left behind or will have to reimplement themself, depending on how much we get into core. For example, the smtp module could just be a UI to configure the symfony smtp transport, or we have a new flexible UI in core or a new contrib module that supports many different transports.
I still think my idea in #83 might work, basically a feature flag, and each site can decide on its own, at least for core and collaborating contrib modules, when they switch to the new system. Once the flag is switched, all modules that respect the feature flag will switch over to the new API, any that don't will either still use hook_mail() (which still works until we rip it out in D12 or whatever) or will already start to use the new API whenever they feel like it.
Fully transparent BC seems pretty much impossible, especially for 4, in the end, either you use the old mail API to send the mail or the new, there's no both.
> This issue is starting to veer into #1346036: Add an alternative to hook_mail() and deprecate it, where I posted a detailed plan for deprecating hook_mail().
I think that was always the scope here. In my opinion, I don't think we need to have/enforce the concept of a mail builder aka hook_mail(), in a way, the scope of this issue in my opinion is to replace the current mail plugins and just drop the concept of hook_mail(). Your module could then just rewrite EntityMail::mail(), and instead of bypassing MailManager and calling the mail plugin directly, you'd create an Email object (either the one from Symfony or ours, to be defined). Everything else would stay as-is.
IMHO, mail builders/config entities to manage mails are useful many cases of e-mails where e-mails should be configurable things, user mails are the perfect example for that. But there are also many cases that don't fit into that, from very basic one-off custom e-mails to things like Simplenews on the other end of the spectrum, which already has its own email management.
In short, that other issue could, instead of trying to replace hook_mail(), become a new, optional API on top of the new mail API that you can use if it fits your use cases (configurable/translatable/extensible mails).
- π¬π§United Kingdom catch
I would say technically, we don't need to guarantee that any particular e-mail goes into hook_mail_alter(), e.g.:
10.1.x - user registration e-mails are in hook_mail
10.2.x - user registration e-mail has moved to the new system and no longer appears in hook_mail_alter().If you're specifically altering the user registration e-mail, then yes this is going to break your module, but that's the same for form alter, preprocess etc. which we allow changes for here: https://www.drupal.org/about/core/policies/core-change-policies/frontend... β . There used to be a more generic section about alter hooks in the main bc policy but seems it got moved to the front end one, will look at adding one back.
If we convert core e-mails one by one over time without a feature flag, we're not changing the actual API of hook_mail_alter(), it just won't get that particular e-mail any more. It's like if we changed an entity-specific form to a generic entity-wide one and the form ID changed, the alter wouldn't fire for that either.
I'm not sure how a feature flag would work with contrib. Let's say core maintains both hook_mail() and new versions of every e-mail, and 15 contrib modules also maintain both hook_mail() and new versions of every e-mail. And 10 modules implement both hook_mail_alter() and whatever the new alter system is for a few different e-mails (or to consistently alter every e-mail). But some contrib modules will only have hook_mail() e-mails, and some contrib modules will only have new-style e-mails - what happens with those?
Feels like we're talking about something similar to π Implement utility method for invoking backward compatible code Fixed except with a feature flag rather than by version, and then all the 'well behaved' modules will send new-style or old-style based on the flag and everything else uses only new or old.
I'm not sure if 'everyone has to maintain two versions of an e-mail and site owners have to decide which one they prefer' is going to be less work for everyone than 'contrib and custom modules have to check their hook_mail_alter() implementations every six months in case an e-mail has been converted.
However I don't maintain any contrib modules really, so pinch of salt and everything.
- π¬π§United Kingdom joachim
> Email transports (back ends) are already defined by symfony. We can extend them with a config GUI (maybe in contrib?)
> IMHO, mail builders/config entities to manage mails are useful many cases of e-mails where e-mails should be configurable things, user mails are the perfect example for that. But there are also many cases that don't fit into that, from very basic one-off custom e-mails to things like Simplenews on the other end of the spectrum, which already has its own email management.
Agreed that not every module that sends mails needs config entities - it would be overkill in cases like simplenews and webform notifications.
But I really think the config UI should be in core - core needs it for User module emails for one thing. If we don't provide it in core, contrib will reinvent that wheel over and over again. And as demonstrated by Mail module, making config and a UI for this is fairly straight-forward.
- πΊπΈUnited States DamienMcKenna NH, USA
Regarding #90, so long as change notices are written that document the changes necessary for contrib code and custom code, I think that would be completely fine.
- π¨πSwitzerland berdir Switzerland
> I would say technically, we don't need to guarantee that any particular e-mail goes into hook_mail_alter(), e.g.:
I was going to say hook_mail_alter() is more about custom stuff, like converting user mails to HTML, but there are actually quite a few contrib implementations, there's modules like https://www.drupal.org/project/bcc β for example.
The feature flag idea isn't just about the alter hook though. Many sites today use modules like smtp, swiftmailer and so on to send HTML through SMTP and other transports. If we change user mails in 10.x to be sent through the new API, they will likely no longer be sent at all. This is possibly not something that sites think of when testing. Is that really OK? I'm not sure.
> But some contrib modules will only have hook_mail() e-mails, and some contrib modules will only have new-style e-mails - what happens with those?
We can't force contrib to to follow the pattern, but we can do it for core and we can provide a change record that does it in the same way. I would expect quite a few contrib modules will follow that, especially big ones that send a lot of critical mails like commerce and webform. Yes, it's similar to the issue you mentioned, it's also not just a feature flag but also allows for core BC, if a feature flag/config key isn't active, it might also be an older core version, and then we can fall back to hook_mail. Especially as I expect this will not get in with 10.3 but 10.3 or 10.4 even?
Realistically, most contrib won't immediately switch over, many smaller ones struggle to have the changes committed in time for a new major version, not too concerned about that. So when core starts to switch over with a feature flag and for example has a requirements message about this but for now continues to work as-is then both sites and contrib maintainers gain some time. Sites will need a replacement/new version of modules like smtp/swiftmailer that allow to configure the required transports, that will need to exist first. (As maintainer of mailsystem, I'm open to getting in patches that at least add some basic UI options, whether or not mailsystem will continue to exist or not, possibly not, but there are some features that like theme selection which might or might not get into core).
> But I really think the config UI should be in core - core needs it for User module emails for one thing. If we don't provide it in core, contrib will reinvent that wheel over and over again. And as demonstrated by Mail module, making config and a UI for this is fairly straight-forward.
I agree with that, having something generic for what user module does could be useful in core, I definitely built very similar UI's in custom modules too. But I do think that it can easily be a separate issue once the new API is in place and then build on top of that. We shouldn't have to touch the configuration and UI of user mail configuration for the low-level API transformation, I'd expect we just convert _user_mail_notify() + hook_mail() into a new service, with the feature switch.
What I would suggest as next step is then an implementation issue to starts to define the mailer service(s) and APIs and we can start to implement at least a few core implementations for example user and contact. Then we can also look at the changes and discuss just how much standardized builder API (aka hook_mail()) we need or if we're ok with one-off services for that in those modules.
- heddn Nicaragua
We mention swiftmailer, but π¬ Conflicts with drupal/core-recommended 10.1.x-dev requires egulias/email-validator ~4.0.1 Active is kinda requiring an upgrade to symfony_mailer as of Drupal 10.1. Does that change any of this discussion?
- π¨πSwitzerland znerol
Opened π [PP-1] Add symfony mailer transports to DIC Postponed which demonstrates how I think we should integrate the transport layer (i.e, mail delivery, nothing about mail building yet).
- π¨πSwitzerland berdir Switzerland
> We mention swiftmailer, but #3359916: Conflicts with drupal/core-recommended 10.1.x-dev requires egulias/email-validator ~4.0.1 is kinda requiring an upgrade to symfony_mailer as of Drupal 10.1. Does that change any of this discussion?
swiftmailer was just an example for one of the various mail plugin modules that are currently used. FWIW, the conflict is only with drupal/core-recommended, not drupal/core.
- π¬π§United Kingdom adamps
> Fully transparent BC seems pretty much impossible, especially for 4, in the end, either you use the old mail API to send the mail or the new, there's no both.
>I'm not sure how a feature flag would work with contrib.Drupal Symfony Mailer module provides a lot of flexibility for handling the transition with good BC:
- Global switch for which mailsystem to use (enable Drupal Symfony Mailer = use the new system). We have a
MailManagerReplacement
mapper that allows the new mailsystem to support the old API, includinghook_mail_alter()
. So even without any changes to contrib modules, you can switch the global switch. The transports change, which may require action from site-builders (however automatic migration is a possibility). The actual emails should stay exactly the same, giving full BC to modules and hooks. - Modules may choose to support both APIs (core does, contrib can choose). Switching API may change email configuration and contents if the module took the chance to adopt a new UI or new features such as HTML support. In this case we should have a per-module switch for which API to use, to allows site-builders to choose when to change (given that a contrib module might introduce support for the new API in a patch version).
- OR Contrib modules may create a new major version that adds support for the new API and drops support for the old. In this case, the new major version should have a
hook_requirements()
that the new mailsystem is enabled. Site-builders would upgrade to the new major version at the same time as switching on the new mailsystem. - An easy option to provide the global switch is to put the new mailsystem in a module, which could be experimental at first. Contrib module releases that require the new mailsystem can simply add a dependency on the new module.
- We can deprecate and remove the old mailsystem, whilst keeping legacy support for the old API for an extra Drupal major version to ease transition.
- Global switch for which mailsystem to use (enable Drupal Symfony Mailer = use the new system). We have a
- π¬π§United Kingdom adamps
> IMHO, mail builders/config entities to manage mails are useful many cases of e-mails where e-mails should be configurable things, user mails are the perfect example for that. But there are also many cases that don't fit into that, from very basic one-off custom e-mails...
>Then we can also look at the changes and discuss just how much standardized builder API (aka hook_mail()) we need or if we're ok with one-off services for that in those modules.The MailerBuilder plug-ins in Drupal Symfony Mailer seem to work well, for example:
- Call the building code in a new render context to avoid leakage. We can write code to build a mail directly with causing this problem provided we do it carefully, leaving all rendering, token replacement, etc to occur inside the mailer.
- Encapsulate the code - a site builder can swap the implementation using a hook.
- Plug-in annotation provides metadata, e.g. human-readable names for different mail-keys for use the config UI.
- Saves creating an extra service for each module that sends emails:
_user_mail_notify('password_reset', $account)
becomes simply\Drupal::mailer()->mail('user', 'password_reset', ['user' => $account])
or$this->mailer()
.
However I agree they shouldn't be compulsory.
- π¬π§United Kingdom adamps
> Simplenews on the other end of the spectrum, which already has its own email management.
With Drupal Symfony Mailer, this custom email management becomes redundant. See π Use Symfony Mailer API to send mails Needs work which removes 2000 lines of code. Similarly commerce has invented its own system (less complex) that could be removed.
> But I really think the config UI should be in core - core needs it for User module emails for one thing. If we don't provide it in core, contrib will reinvent that wheel over and over again. And as demonstrated by Mail module, making config and a UI for this is fairly straight-forward.
Drupal Symfony Mailer also has a config UI which supports approx 20 fields (not just body/subject) which can be set for specific mails or all. Once we have the base mail system in place then we can look at options for the config UI.
- π¬π§United Kingdom adamps
> What I would suggest as next step is then an implementation issue to starts to define the mailer service(s) and APIs and we can start to implement at least a few core implementations for example user and contact.
I have created β¨ Create new mailer service based on symfony Active . I'm willing to write an initial patch. However first can people provide feedback and give opinion on two questions that I asked.
- π¬π§United Kingdom adamps
I created two more issues β¨ Create a plug-in system to build emails with Symfony Mailer Active and β¨ Create a system for configurable emails in Symfony Mailer Active .
Feedback and comments very welcome please. I've written them based on "Drupal Symfony Mailer" which has been proven - but of course it's just one possible way of doing things. Any other ideas very welcome it's all open for debate.
- π¬π§United Kingdom adamps
I raised 3 more issues: β¨ Migration strategy for moving to Symfony Mailer Active , β¨ Create a BC mapper for MailManager to convert to Symfony Mailer Active , β¨ Change core modules to use Symfony Mailer Active . I'll wait a week or two now to allow people to comment.
- π·π΄Romania claudiu.cristea Arad π·π΄
Don't know if this is relevant but I want o make you aware that Devel module offers a mail file mail catcher, see https://gitlab.com/drupalspoons/devel/-/blob/5.x/src/Plugin/Mail/DevelMa.... As it's a popular module we should be careful not to break its functionality
- π¨πSwitzerland berdir Switzerland
> Don't know if this is relevant but I want to make you aware that Devel module offers a file mail catcher, see https://gitlab.com/drupalspoons/devel/-/blob/5.x/src/Plugin/Mail/DevelMa.... As it's a popular module we should be careful not to break its functionality
Depends on your definition of "breaking". It will not break, it will simply cease to be used, like any other mail plugin using the current mail plugin, the whole concept/plugin system will be deprecated and eventually removed.
_Many_ modules in their current form will either no longer be needed or will need to completely rethink their approach and just become a UI for symfony mail transports for example. And any site that doesn't use the default mail plugin will need to make some adjustments, but we're doing our best to make this as smooth as possible.
In regards to the issues, I'm unsure how much we can split it off and in what form. As there's no guarantee when issues will land, it would for example be pretty weird if the transports are in but nothing is using them. So both from that perspective and also to be able to review and discuss the impact, we might need to split vertically with a minimal feature set instead of horizontally by layer. For example, β¨ Create new mailer service based on symfony Active proposes to add a ton of features to the mail service, like theme switching. Which I find super important and a critical feature of the mailsystem module, But it's also something that both doesn't exist yet in core and is hard to test and review without anything using it.
So IMHO an approach where we do a minimal implementation of the mailer, just as much as we need to use it for the current use cases in core and start to convert them so that we can see how the whole thing will work for example for user module. At first without a BC layer. We can still try to keep different systems/layers and the actual module conversions in separate commits in a merge request with a bit of git rebase -i juggling, Then when we're happy with how the conversions look, we can get the mailer/transport and whatever other classes we need in in their primary issues.
On experimental module vs core I'm unsure. Core would be nicer of course but we would need to figure out how to define an experimental API in the Drupal\Core namespace, I think we haven't done that yet, would probably require input from framework/release maintainers. An experimental module would eventually require a move and deprecation.
- π·π΄Romania claudiu.cristea Arad π·π΄
(...) will need to completely rethink their approach and just become a UI for symfony mail transports
Trying anticipate that, I've proposed upstream a file mailer transport https://github.com/symfony/symfony/pull/50962, unfortunately rejected.
- π¬π§United Kingdom adamps
Trying anticipate that, I've proposed upstream a file mailer transport https://github.com/symfony/symfony/pull/50962, unfortunately rejected.
Shame, but presumably it can live somewhere else?
- π¬π§United Kingdom adamps
@Berdir - yes I see what you mean, it does make sense. So we could make a prototype working system as a vertical split of the existing issues just taking the minimum from each for a working system.
Do you imagine we would actually commit this prototype?
==
Key question from β¨ Create new mailer service based on symfony Active - do you have any thoughts?
Should the new class Email extend the base symfony one, or decorate it (meaning have a class variable $inner containing the base class)?
- Advantage extend: we can pass our class everywhere where the base class is expected.
- Advantage decorate: we can make a clean start, renaming/removing functions or changing their parameters.
- Status changed to Needs review
over 1 year ago 12:52pm 22 August 2023 - π¬π§United Kingdom adamps
OK, I posted a patch on β¨ Change core modules to use Symfony Mailer Active .
It may seem strange to have this without any patch for the underlying services/interfaces. However I feel it gives a pretty good idea how the whole system will look, and it's easy to deduce how the interfaces will be, especially as I wrote really detailed comments.
Key point: I am assuming a new mailer service in Core, with capabilities to send via the old mail service.
Advantage: We can make a clean switch in the core modules to call the new API only. We discard the code that calls the old API and avoid the need for anif (new_mailer)
statement for every bit of code that sends a mail.Then I propose a new issue "Create a new mailer service/API ready for symfony, but still send using the old mail service", which would be a fairly small amount of simple code. Together with the above:
- Core modules use the new API
- with calls to exactly the same
hook_mail()
andhook_mail_alter()
- generating exactly the same mails
- meaning the existing tests all pass without any changes
- so these two could be committed without relying on any other issues.
I can also make a usable prototype in contrib (perhaps v2 of the existing Drupal Symfony Mailer) that replaces the mailer service with one that allows the above code to actually send via Symfony Mailer.
I welcome comments please:
- Comment here about the proposed strategy
- Comment in β¨ Change core modules to use Symfony Mailer Active (can use GitLab) about the details of the API and code changes
- π¨πSwitzerland berdir Switzerland
> Should the new class Email extend the base symfony one, or decorate it (meaning have a class variable $inner containing the base class)?
I don't know. I was hoping that we would be able to stick to using the symfony component as directly as possible without a lot of Drupalism on top, but you made a few good points about problems with that like the strict types that conflict with our translation system.
I think what I'd like to see is that we try to work directly with Symfony as much as possible first and then look at the result and introduce APIs to improve that where we think it's worth doing. Your work in β¨ Change core modules to use Symfony Mailer Active is interesting, but it's not really a vertical split based on what we have, it's more the icing on a multi-layer cake :)
One thing to keep in mind with BC is not just the current state and transition to that new system but also the future. The downside of working directly with certain symfony components like the serializer is that those were some of the most annoying BC issues between D9 and D10, but that was partially because we skipped a major version. If we decorate/use adapters, then we can deal with symfony API changes for example in the e-mail class most likely in nicer ways.
- π¬π§United Kingdom catch
you made a few good points about problems with that like the strict types that conflict with our translation system.
We've tried to persuade Symfony to use Stringable type hints instead of string for validator, but so far that's been rejected. #3255245: [Symfony 6] Revert 3231603 to use our own TranslatorInterface β and especially https://github.com/symfony/symfony/pull/44911 have details. I'm all in favour of them getting even more feedback that preventing the use of Stringable is an unreasonable API limitation so maybe we can have another push via the mail system. However given how that went last time, decorating seems a good way to deal with unreasonable API limitations and future changes that they introduce.
- π¬π§United Kingdom adamps
Great thanks.
it's not really a vertical split based on what we have
True. As I understand it, your vertical split is a working system for one module. The difficulty is that the only issue that is module-specific is β¨ Change core modules to use Symfony Mailer Active so it would mean writing nearly everything. So what I've tried to do is create a strategy that is
1) Easy to commit (relatively speaking) - two manageable size issues together make a significant step forward in a fully-BC way, with the chance to test the future strategy works using contrib.
2) Meaningful to review - that one issue already gives a good idea of the way forward and people can see if they like it.decorating seems a good way
Also we do decorating already in contrib so it seems like the way to go.
- π¬π§United Kingdom adamps
I've updated some of the child issues in response to feedback so far. This leads to simplication and I've added a lot more detailed explanation. Further comments very welcome.
Next step I will aim to create a minimal usable patch as described in #109 - except I can do what Berdir suggests and include only one module, likely user. I created β¨ Minimal usable Symfony Mailer prototype Active .
- π¬π§United Kingdom adamps
First step done!! π Add symfony/mailer into core RTBC = Fixed
Next step: π [PP-1] Add symfony mailer transports to DIC Postponed
- π¬π§United Kingdom adamps
Added 2 new issues to stage 1.
My current idea is to implement features for this initiative if possible using the stage 1 @Mail plug-in. These issues are each fairly simple and self-contained, hopefully easy to commit. At the same time, we can create new releases of Contrib Drupal Symfony Mailer that remove the matching function and use Core instead.
This makes stage 2 more approachable - otherwise it seems like a huge block of code chunks that all depend on each other.
- π¬π§United Kingdom adamps
There are ways in which I feel I can help this initiative:
- I've developed some specialism in Drupal mail. I identified an area that could be improved and did something about it. Drupal Symfony Mailer module can be a template to help add similar features to Core.
- @znerol created some really nice quality patches, and I reviewed them based on what I Iearnt from developing Contrib.
However when it comes to this META issue, I don't feel I'm succeeding to help. It seems that the majority of comments I make and issues I raise meet an objection from @Berdirπ. I'm only on the fringes of the Drupal community and I'll freely admit that I'm not the best one to predict what Core would like. Maybe someone else would like to step in and take the initiative? There's code from DSM, some patches on these issues that I already attempted. Plus I'm still keen to keep an eye on things with reviews based on what I already learnt.
In any case, looking after the mailer in Contrib is already keeping me quite busy, and I can continue to work on it to adapt to what happens in Core.
- π¨πSwitzerland berdir Switzerland
It really wasn't my intention to push you away, I'm sorry about that. I was just trying to help guide it in a direction that I personally think has better chances of getting into core in a reasonable timeframe. And sometimes it might just not match what my idea was, but I think β¨ Framework for testing Symfony Emails Active shows that we are hopefully on the way to common ground on at least some of these issues :)
- π¬π§United Kingdom adamps
Don't worry it's OK - my comment was also a knee-jerk reaction sorry.
However I think there is a good underlying point: it would be great to have a clear direction from Drupal Core "product management" for what the Core committers would accept. Or perhaps we need a team of people willing to work on this initiative with a range of different backgrounds and skills?? Perhaps that team should include a Core committer?
IMHO Stage 1 is succeeding. There is a step-by-step sequence of things we can add to Core that are each fairly small and self-contained. We can then adjust Contrib (DSM, DSM-L and new Mailer Transports module) to match - using what's in Core, and extending it. I guess that's partly why I'm attracted to add more things to this stage, but I'm certainly not fixed on that approach.
Stage 2 is harder for me to see how to move forward. It's a whole new mail system, which while not huge, feels to big for a single issue. Then if we split it, how does half a mail system make any sense? I proposed β¨ Minimal usable Symfony Mailer prototype Active , but I feel it's more of a prototype that something we'd actually commit. Comment #109 describes the minimum thing I can see that makes a useable system - two issues, each quite big but not huge, they feel possible to commit.
My time is already quite full with Contrib DSM. Another way we could move forward is to get some key people from Core to become advisors/reviews for that, and we can move it in the direction that would be attractive to Core. Contrib is much more flexible than Core, so it might be easier to get the shape mostly right there. Then we can come back to offer Core something that's already close to being acceptable, and with validated on 1000s of live sites.
- π¨πSwitzerland berdir Switzerland
Agreed, stage 2 is quite hard. And I think we agree on π [PP-1] Add symfony mailer transports to DIC Postponed being the next main step, but that too is fairly complex already, don't feel like I can contribute much there, no clear opinion for once ;)
I pinged @longwave in Slack and he'll bring it up with the other committers as well. I can imagine that @alexpott or @catch might have opinions on the DIC issue.
β¨ Minimal usable Symfony Mailer prototype Active does seem like a good place to then experiment on stage 2, I'll comment a bit more there.
- π¬π§United Kingdom longwave UK
Discussed the above with @alexpott, @larowlan, @catch and @lauriii. We previously agreed that the mail system does indeed need a complete overhaul, and from prior discussions nobody can see a way of doing this with full backward compatibility. We are generally excited about the current approach and the progress made so far. However, we noted that to date the problem has been approached from a mostly technical point of view, and we should first of all take a step back and define our goals here.
From a product perspective, we have questions such as:
- What email features do users expect Drupal to have?
- What features do our competitors provide?
- What features do contrib modules provide that we should move into core?
- What features should remain in contrib?
Tagging for product manager review to try and define where we should be going with this. It seems pretty clear to me that one goal should be "HTML emails out of the box" as we are providing a complete rewrite of the mail system, but other than that I'm not 100% sure on decisions we have made from a product point of view.
I think support for file attachments would be good as well, if that's not covered by "HTML emails out of the box".
- π¬π§United Kingdom adamps
Thanks @longwave.
It seems pretty clear to me that one goal should be "HTML emails out of the box" as we are providing a complete rewrite of the mail system, but other than that I'm not 100% sure on decisions we have made from a product point of view.
Mostly so far we've not made product decisions. We've made a 99.9% certain technical decision is to use Symfony Mailer. Technically it seems like a very strong choice, and I don't see how it would limit our choices for product features. Symfony includes HTML, attachments and many other things. Even if we want a feature it doesn't have, we should be able to add it.
In Contrib there is Drupal Symfony Mailer β . It provides two big features that don't necessarily belong in Core:
- "Mailer Transport" GUI for selecting and configure the symfony transport (including a page to send a test email)
- "Mailer Policy" GUI - fully flexible configurable mails: body, subject, addresses, headers, etc.
Some questions it would be useful to answer:
- The rough consensus of comments so far is that the 2 above GUIs should remain in Contrib. It would be useful to have clarification of that.
- There will be a lot of detailed features questions, for example should Core support setting the theme used for sending emails? (Currently no - it's done in mailsystem.) What would be the mechanism to answer these?
- On the boundary of product management and technical there will be some "architecture" type questions such as: How will the new mail system use templates, themes/libraries, tokens, etc? What would be the mechanism to answer these?
- ππΊHungary mxr576 Hungary
It seems pretty clear to me that one goal should be "HTML emails out of the box" as we are providing a complete rewrite of the mail system,
Just to connect some dots, I'd like to mention here that I read a different opinion with concerns explained from @berdir in #3392879-5: Add support for HTML email to Symfony Mailer plug-in β and #3392879-7: Add support for HTML email to Symfony Mailer plug-in β
- π¬π§United Kingdom adamps
Just to connect some dots, I'd like to mention here that I read a different opinion with concerns explained from @berdir
I believe the concerns from @Berdir are about whether to support HTML in the "Symfony Mailer plug-in", meaning the @Mail plug-in which was created in π Add symfony/mailer into core RTBC . I'm fairly sure that we all want to support HTML in the new mail system API which will eventually be created in β¨ Create new mailer service based on symfony Active .
- ππΊHungary mxr576 Hungary
ahh, mindblowing... sorry for mixing things. Increasing my followed Symfony Mailer-related issues with one more... =]
- Status changed to Needs work
about 1 year ago 11:56pm 10 November 2023 The Needs Review Queue Bot β tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request β . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
- Status changed to Needs review
about 1 year ago 12:02am 11 November 2023 - π¨πSwitzerland znerol
While the mail delivery part is ready and waiting for review in π [PP-1] Add symfony mailer transports to DIC Postponed , filed π Introduce theme negotiator for mail and use it in mail manager Active as a very small first step for the mail building part. Theme switching is necessary anyway, so I figure we could build that out in a way which is beneficial for the current core and contrib infrastructure as well.