I do not need help with this issue.
Chaising head 📌 [ignore] Convert everything everywhere all at once Active .
Re #141.1 The fromString() method is still available from the full-blown Symfony mailer Transport facade. It can be used by contrib and custom code to parse string DSNs. Search for MAILER_DSN in the examples sandbox for pointers on how to do that. Core stores the DSN in its structured form since
✨
Use structured DSN instead of URI in system.mail mailer_dsn
Fixed
. And
📌
Tighten config validation schema of system.mail mailer_dsn
Fixed
made it pass config validation. Also additional schemes can be brought in by custom and contrib modules for third-party transports. Hence, core does not have any need for a reimplementation of the fromString() (and consequently the parseDSN()) method. Its maintenance cost should be left with Symfony developers.
Re #141.2 Contrib and custom code certainly can do whatever fits the use case at hand. The recommended way to customize transport instantiation is to decorate the factory.
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.
Re #6, I cannot find the core issue. Did you file that? Would you mind linking to it from here?
Interesting. Do you have any pointers for a repro? E.g., does that happen when requesting an non-existing service inside a kernel test?
For the same reasons as in ✨ Add Alpha level Experimental Package Manager module Needs review , this issue likely neither needs a CR nor a release note snipped. Added a note about that to the issue summary.
Re #131.1:
The Symfony mailer Transport facade is now gone.
Re #131.2:
I think we have to accept that we disagree in that regard. I'd like to remind people here, that we seem to have reached consensus on that in Vienna last year 🌱 [META] Adopt the symfony mailer component Needs review . Core should just stick with one transport, but it should not get in the way of contrib / custom code if they replace it with something fancier.
Re #133:
I propose that we postpone adding those factories to a follow-up. An initial version doesn't require auto detection of available transports.
Updated the sandbox for new TransportServiceFactory.
Thanks for the suggestions. For this iteration I focused on the Symfony mailer Transport facade which rightfully has been the subject of quite some discussions in this issue.
Thus, I reworked TransportFactoryAdapter into TransportServiceFactory and removed its dependency on the Symfony mailer Transport facade. That unlocks a couple of interesting simplifications:
- We can use the
AutowireIteratorattribute to collect services tagged withmailer.transport_factorydirectly from the constructor. - That renders
TransportFactoryCollectionsuperflous. - In order to avoid the Symfony mailer Transport facade completely (no decoration necessary) it is enough to copy the simple
fromDsnObject()method into a smallTransportServiceFactoryTraitto make it reusable by transport service factory implementations.
Adapted the issue summary with the new service structure.
Removed the @logger.channel.mail argument in the abstract transport service definition in order to avoid
🐛
Core Symfony Mailer throws error on transport shutdown
Active
.
I'm tentatively removing the needs test tag here. In order to reliably cover that corner case we'd need to add an end-to-end test which successfully sends an email message over SMTP and then asserts that there were no errors after service destruction. This is neither possible with unit tests nor with kernel tests.
Given that we are dealing with an experimental plugin here, I'd say that we can save ourselves the effort to add a test for this.
znerol → changed the visibility of the branch 3420372-core-symfony-mailer to active.
znerol → changed the visibility of the branch 3420372-add-mailer-development-setting to hidden.
Some manual testing led me to conclude that the debug messages aren't of any use, not even in development mode. Exposing a switch in the UI is overkill. Hence, I think it is better to simply remove the logger argument from the call into Transport::getDefaultFactories().
I found another place where we do the same (i.e., not passing the logger into a symfony component):
See the service definition for the Symfony router_listener, especially the last argument:
router_listener:
class: Symfony\Component\HttpKernel\EventListener\RouterListener
arguments: ['@router', '@request_stack', '@router.request_context', NULL]
Setting this to needs work. I hope that somebody will find the time to adapt the original PR, I'll stick around for rewiews.
znerol → changed the visibility of the branch 3420372-core-symfony-mailer to hidden.
znerol → changed the visibility of the branch 3379794-pp-1-add-symfony to hidden.
Thank you for the feedback.
Re #124
I feel that policy discussions merit their own issues. I did observe the practice in Drupal core to seek policy decisions in issues with the title prefix "[policy, no patch]". I like that approach because it gives non-code decisions the weight and visibility they deserve.
Regarding the issue summary. I removed the user stories and replaced them with a version of the statement from #116. Additionally I did reword the issue summary a little bit around the sandbox link.
I did update the list of services in order to reflect the new approach in !9831 and did hide the older MR.
Re #125
The Transport::getDefaultFactories() started out as a private method in the mailer component. It has been made public in order to satisfy the needs of projects which reuse the mailer component without a container (see symfony#35469. Drupal comes with a container. Prepopulating the list of transport factories by calling into Transport::getDefaultFactories() would lead to a weird situation where some transport factories would appear automagically under special circumstances (i.e., if their dependencies happen to be installed by composer), and others need to be registered explicitly. This inconsistency also would lead to a situation where some factories can be modified, removed or decorated (because they were explicitly registered in the container) and others not. In my opinion, this is a no-go DX wise.
See also PHP issue 12097. Since OtLog uses strict types, there will be no automated cast from Stringable to string.
There is some precedent in core for services without a service name. The only way to reference this services is the fully qualified interface name.
Opened a second MR 9831 in order to explore the fqcn naming pattern. Also updated the sandbox module. That one should now work with both branches.
Sorry, forgot to answer that one:
- The example code seems to use the API from this issue directly.
- The example code seems to have the problems that I describe in #114.
This issue and also many issues in the contrib projects which integrate symfony mailer show that it isn't too easy to understand the architecture and integration options of the upstream component. The example code validates the approach taken here. It isn't supposed to be the reference for the resulting product.
Thank you for your feedback.
The IS states that developers can write code in 7 specific ways. For me this implies that code would use the API from this issue directly.
I revised the issue summary slightly to emphasize that this is bleeding-edge stuff. In order to spec out the scope, I did choose a common user story template. I can switch to another form of scope definition if the current one is not clear enough.
- The example code seems to use the API from this issue directly.
- The example code seems to have the problems that I describe in #114.
2) I'm not sure you answered my point about whether mailer.mailer is definitely the right name for the delivery layer. Do you have a proposal for name of the layer above that is the actual mailer that people should use?
There is some precedent in core for services without a service name. The only way to reference this services is the fully qualified interface name. Recent examples include the sdc ComponentLoader or the CsrfExceptionSubscriber. I'm going to try out whether this is possible for the mailer as well.
Well my doubts are mostly about TransportFactoryAdapter and ConfiguredTransportFactoryInterface which I guess aren't from symfony mailer, but our own adaption?? I feel that if we instead adapt in a slightly different way, then the result could be much more use to contrib.
See the comment on the GitLab MR. In summary, contrib and custom code wishing to customize the way transport(s) are configured is not supposed to reuse TransportFactoryAdapter. Instead such modules are supposed to supply their own implementation of ConfiguredTransportFactoryInterface.
Thank you for the feedback.
Re #114
- module+key to help hooks identify mails (we could combine them into a single string)
Symfony Mailer allows for tags and metadata attached to messages. These can be used to identify messages from within event subscribers. Some third party transports even pass them to external mail services, where they can be used further for grouping/routing/accounting etc.
- encoding key information into 'params' and then building the email from these (allows hooks to alter the params, or code can replace the building code entirely)
- building the email in a callback function to switch render context so avoiding pollution (and we can add switching of language and user)
There are two activities when it comes to send transactional mail from Drupal:
# Building mails
# Delivering mails
This issue is about the second activity. It is clear that the mail building part will need another round of exploration and experimentation to get to something simple and useful. But it is out of scope for this issue.
From the remaining comments I gather, that you are very concerned about the lower level part (i.e., the mail delivery) to be exposed in the container. Let me point out, that we do that a lot in core. For example, there is a database service giving developers raw query access to the database. But, it is much more safe and convenient to create entities and use entity queries. As a result, nobody uses the database directly for day-to-day tasks, even though it isn't explicitly marked as @internal. Hence, we shouldn't be concerned to expose mailer.mailer in the DIC.
Re #115 I'd rather keep things simple here. The mailer module is marked experimental. It is possible (and expected) that the API can change. If truly required, then the API surface and the inner workings can be made more sophisticated. But for the beginning, I prefer to follow closely what symfony mailer offers out of the box.
Thanks, I agree with the approach. Left a small note in the MR.
A simple test case could go into SymfonyMailerTest.php