Account created on 8 June 2006, over 17 years ago
#

Merge Requests

More

Recent comments

πŸ‡¨πŸ‡­Switzerland znerol

Reroll succeeded without conflicts.

πŸ‡¨πŸ‡­Switzerland znerol

I suggest to move the logic into a response event subscriber instead.

πŸ‡¨πŸ‡­Switzerland znerol

This includes #3023104. As one of the authors of the cache policy mechanism, I'd be disappointed if this approach would make it into core. Citing from #3023104-4: Introduce "Vary" page cache response policy β†’ :

Regrettably this approach clearly violates the ResponsePolicyInterface contract. While not stated explicitly in the documentation, the check() method is certainly not expected to actually change the response.

πŸ‡¨πŸ‡­Switzerland znerol

On the other hand, there is a good reason for this patch here to actually use Transport::getDefaultFactories(): Backward compatibility. Sticking with Transport::getDefaultFactories() avoids breaking mail delivery on existing sites which are currently using a third-party transport and relying on the fact that the static part of Transport API is magically registering factories.

πŸ‡¨πŸ‡­Switzerland znerol

Key thing is that its not on the deprecation path just yet,

The reason why I'm suggesting to avoid Transport::getDefaultFactories() has nothing to do with deprecation. At least I am not aware of any plans to deprecated it.

πŸ‡¨πŸ‡­Switzerland znerol

Transport::getDefaultFactories() is another shortcut which will fall on our feet sooner or later. I humbly recommend to avoid that as well.

πŸ‡¨πŸ‡­Switzerland znerol

Why does symfony/mailer make it _so_ hard to customize things. Why even support configurable authenticators when it's impossible to set them without copy pasting 200 lines of code?

Unfortunately the Transport class exposes two separate and very different API entry points. In my opinion the dividing line is public static methods vs non-static ones. The former (static methods, i.e. fromDsn(), fromDsns() and getDefaultFactories()) simplifies transport construction for very small projects lacking a DI container. The latter provides all the flexibility (including custom factories) which come in handy at some point in larger code bases.

Let's put it that way: In projects with a DI container, any code path going through any of the public static methods of the Transport class is taking shortcuts and as a result is missing important extension points.

That should be discussed over in πŸ“Œ [PP-1] Add symfony mailer transports to DIC Postponed though.

πŸ‡¨πŸ‡­Switzerland znerol

I worked around this issue by deleting all report data, uninstalling commerce reports and reinstalling it again. After that I did regenerate all report data.

Unfortunately I cannot pinpoint the exact circumstances which are triggering the problem. It was reproducible though (the status report was always there when I upgraded from the same database snapshot).

πŸ‡¨πŸ‡­Switzerland znerol

Thanks @Prashant.c, back to RTBC (identical to #24).

πŸ‡¨πŸ‡­Switzerland znerol

Quick status update:

This issue still needs feedback from framework managers. Would be cool if we could make this happen in 10.3.

πŸ‡¨πŸ‡­Switzerland znerol

I removed the services.yml example from the CR. The !tagged_iterator notation still doesn't work. This issue here only changed the container array dumper/loader and not the yaml part (see also #38).

Attempting to use the !tagged_iterator notation results in the following exception:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The file "core/modules/mailer/mailer.services.yml" does not contain valid YAML: Tags support is not enabled. Enable the "Yaml::PARSE_CUSTOM_TAGS" flag to use "!tagged_iterator" at line 42 (near "arguments: [!tagged_iterator mailer.transport_factory]"). in Drupal\Core\DependencyInjection\YamlFileLoader->loadFile() (line 426 of /home/lo/src/drupal/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php).

Drupal\Core\DependencyInjection\YamlFileLoader->loadFile('core/modules/mailer/mailer.services.yml') (Line: 71)
Drupal\Core\DependencyInjection\YamlFileLoader->load('core/modules/mailer/mailer.services.yml') (Line: 1303)
Drupal\Core\DrupalKernel->compileContainer() (Line: 941)
Drupal\Core\DrupalKernel->initializeContainer() (Line: 496)
Drupal\Core\DrupalKernel->boot() (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
require('/home/lo/src/drupal/index.php') (Line: 48)
πŸ‡¨πŸ‡­Switzerland znerol

Issue now targets 10.3, added a dedicated change record now.

πŸ‡¨πŸ‡­Switzerland znerol

I think the following is happening:

address_update_9201() tries to find all field instances using getFieldMapByFieldType().

  $entity_field_manager = \Drupal::service('entity_field.manager');
  $entity_field_map = $entity_field_manager->getFieldMapByFieldType('address');

However, that method only reports fields on fieldable entity types. Regrettably, the commerce report entity type is not fieldable, and as a result address_update_9201() fails to update the field definitions on the Order Report bundle.

πŸ‡¨πŸ‡­Switzerland znerol

OTOH there's SessionManagerInterface which can be extended with finalizeLogin() and finalizeLogout() methods

I'd prefer if SessionManagerInterface will go away in the long run. All of the remaining code in SessionManager is supposed to be extracted to session handler decorators or even to upstream.

πŸ‡¨πŸ‡­Switzerland znerol

Oh, I might have overwritten work done by @voleger and @dww by basing off my changes on an outdated local branch. Let me fix that.

πŸ‡¨πŸ‡­Switzerland znerol

Fine to self RTBC

Thanks @larowlan. Done.

πŸ‡¨πŸ‡­Switzerland znerol
πŸ‡¨πŸ‡­Switzerland znerol

znerol β†’ created an issue.

πŸ‡¨πŸ‡­Switzerland znerol

I'm not the one to decide. And my plan (or better said my intention) doesn't count anything unless it gets traction in the community. I recently tagged πŸ“Œ [PP-1] Add symfony mailer transports to DIC Postponed with Needs framework manager review in the hope to gather some additional feedback from those folks who will eventually have to weight in on the approach.

πŸ‡¨πŸ‡­Switzerland znerol

Dependency injection has been left out intentionally from the original issue (see #3165762-30: Add symfony/mailer into core β†’ for the reasoning). There is an open issue which tries to introduce the services properly in πŸ“Œ [PP-1] Add symfony mailer transports to DIC Postponed (that doesn't currently touch the experimental mail plugin though).

My plan was to add the plugin to give people something to play with. I wouldn't recommend to extend the experimental plugin any further, or even use it in production. Instead, I suggest that we define and implement a new API based on the Symfony mailer component and then deprecate the old mail plugin system (see also #1803948-81: [META] Adopt the symfony mailer component β†’ for more details).

πŸ‡¨πŸ‡­Switzerland znerol

With the current MR, I'm proposing an approach which is analogous to how the http-foundation component is being integrated. Drupal provides abstractions covering a wide range of use cases typical for application code (forms, entity displays, render arrays). All of those abstractions end up generating a Symfony Response representing headers and content sent to the user-agent.

Application code rarely needs to construct a Response directly. But since the lower layers do not actually have a reason to care about how a response is generated, Drupal doesn't prevent controllers to do exactly that.

Drupal also fires http-foundation events (e.g., RequestEvent). As a result it is possible to directly reuse Symfony RouterListener and other request subscribers even though route storage in Symfony is completely different than in Drupal.

The Symfony http-foundation component is being integrated into Drupal in a way which provides a convenient interface for application code and at the same time provides interoperability with existing Symfony and third-party components.

It is worth thinking of the Email class as a "cousin" of the Response class. Both represent the result of an action and both are responsible to convert it to a representation understood by a user-agent.

Once the mailer component integration is completed, it will rarely be necessary to construct Email instances directly. Instead, there will likely be abstractions in place (provided by core, contrib or custom code) covering a wide range of use cases typical for application code. But the lower layers (i.e., the mailer and the transport(s)) do not actually have a reason to care about how an e-mail is generated.

Abstractions are useful for the mail building part of the upcoming mailer implementation, just as abstractions are useful for the response building part of the existing http controller architecture.

It is important to understand that this MR targets the mail delivery part of the upcoming mailer implementation. This roughly compares to the lower layers of http-foundation. E.g., the http stack middlewares (page cache, session, etc.) and many of the response event subscribers.

I do not know of any Drupal issue which would be easier to resolve if the lower layers of http-foundation (including Response) would be completely hidden behind an abstraction. Even cache metadata bubbling has been solved in a way which is 100% compatible with the Symfony http-foundation component.

Consequently, I do not see any point in abstracting away access to the Email class or the MailerInterface (as suggested in #98). In fact, I am certain that the maintenance costs of such an abstraction would far outweigh any benefits.

Core developer resources are scarce. This MR proposes an approach which is unlikely to cause extensive maintenance overhead. At the same time, it ensures interoperability with additional Symfony and third-party components. All of this is achieved by following the successful integration pattern of the http-foundation component.

Tagging with Needs framework manager review in order to get a sign-off regarding the approach.

πŸ‡¨πŸ‡­Switzerland znerol

It looks like parse_url() accepts a wide range of characters in the host part which are clearly not allowed according to the RFC. E.g., non-ascii letters, the ampersand, colons outside of brackets, etc. However, it replaces control characters with an underscore (including newlines).

Hence, let's reuse the regex from the label data type for the host field.

πŸ‡¨πŸ‡­Switzerland znerol

There is also RFC 6874 specifying the correct syntax for IPv6 zone ids.

πŸ‡¨πŸ‡­Switzerland znerol

According to RFC 3986 Appendix A, syntax validation of the scheme key seems possible with a regex.

ABNF for scheme is:

   scheme        = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )

Translated to a regex: /[a-z][a-z0-9+\-\.]*/i

The host part seems tricky just with a regex. It may be an IP address literal or alternatively it may be a string of alphanumeric plus quite some punctuation characters. The host part may even contain percent escaped characters. Also the IP address literals are allowed in numerous different formats.

I think that for the host field it would be best to implement a custom validator.

There is no restriction for the user and password fields. The parse_url and urldecode (as used by DSN::fromString()) happily accept various control characters (including "%00"). Remote systems probably wouldn't accept user names and passwords with newlines and tabs, but from a spec point of view, such strings are acceptable.

Same for the option values. They actually can contain anything.

πŸ‡¨πŸ‡­Switzerland znerol

Setting status to NR for the updated change record.

πŸ‡¨πŸ‡­Switzerland znerol

Thanks to the fact that the config schema matches exactly the parameter names of the Dsn constructor, we actually can unpack the config array directly into the constructor parameters.

πŸ‡¨πŸ‡­Switzerland znerol

While maybe convenient, I think that #117 will create more problems than it solves.

In Drupal 7, application code modified the $_SESSION superglobal directly. Every piece of code which accessed it was guaranteed to work with the same data. However, using the $_SESSION directly breaks encapsulation and makes testing difficult. Missing test coverage was probably one of the reasons why πŸ› Feature "Remember the last selection" for views exposed filters doesn't work anymore Fixed went unnoticed for quite a long time.

In times where $_SESSION was used exclusively, the responsibility to populate and store its contents was SessionManager.php alone (or session.inc in D7). Now that almost all core code is refactored to use the Request::getSession(), there are additional code paths involved in passing data form the PHP session to the places where its used and back again.

Basically every piece of code which creates, clones, duplicates a request also has the responsibility to copy the session. It is true that many of those are located in test cases, but some can be found in production code paths as well. The latter have the potential to introduce subtle bugs if they do not propagate the session. When everybody used $_SESSION this wasn't an issue. Nowadays it is.

I fear that installing a request factory in the test system will hide those potential bugs. Code which fabricates a request in order to pass that into non-trivial code paths without propagating the session will seemingly behave correctly in tests (because of the request factory) and will produce invalid requests (i.e., lacking a session) in production.

πŸ‡¨πŸ‡­Switzerland znerol

Filed πŸ“Œ Add Psr\EventDispatcher\EventDispatcherInterface alias to core services Needs review , with that one we could use autowiring for mailer.transport_factory.abstract and mailer.mailer.

πŸ‡¨πŸ‡­Switzerland znerol

Adding this to the meta issue #3050720

πŸ‡¨πŸ‡­Switzerland znerol

@borisson_ true. Do you think this could go into a follow-up or is that a requirement for an initial iteration?

πŸ‡¨πŸ‡­Switzerland znerol

Switched on Require review and approval to be added to this guide’s menu.

πŸ‡¨πŸ‡­Switzerland znerol

There should be some kind of factory in the container that produces Mailer objects, such that its arguments are autowired properly. The method I used for contrib Symfony mailer was a non shared service.

Thats the bit I do not understand. Why do you think mailer.mailer must be marked with shared: false? Upstream doesn't seem to dot that.

πŸ‡¨πŸ‡­Switzerland znerol

Still exploring the entry point. Materialized the research from #88 into this PR. The entry point to the mail deliver layer is now @Symfony\Component\Mailer\MailerInterface.

While looking for ideas on how to go about @Berdir questions regarding experimental modules, I looked around in sdc. That made me realize, that we can now rely on service aliases. That is especially cool for the optional dependencies of mailer.transport_factory.abstract and mailer.mailer:

  mailer.transport_factory.abstract:
    abstract: true
    class: Symfony\Component\Mailer\Transport\AbstractTransportFactory
    arguments:
      - '@Symfony\Contracts\EventDispatcher\EventDispatcherInterface'
      - '@?Symfony\Contracts\HttpClient\HttpClientInterface'
      - '@logger.channel.mail'
...
  mailer.mailer:
    class: Symfony\Component\Mailer\Mailer
    arguments:
      - '@Symfony\Component\Mailer\Transport\TransportInterface'
      - '@?Symfony\Component\Messenger\MessageBusInterface'
      - '@Symfony\Contracts\EventDispatcher\EventDispatcherInterface'
  Symfony\Component\Mailer\MailerInterface: '@mailer.mailer'

We do not need to standardize a service name here for neither the HttpClientInterface nor the MessageBusInterface. If any module introduces one of them, it is picked up automatically.

Updated the issue summary for #88.

πŸ‡¨πŸ‡­Switzerland znerol

Done. Honestly, I just copied the sm_test_transport module.

Note that there is no clear naming pattern in existing transports. The name of the doctrine transport module is sm_transport_doctrine. Maybe something to think about before merging.

πŸ‡¨πŸ‡­Switzerland znerol

Wouldn't this then make life in contrib easier? Otherwise I think you will have to override this entirely and provide multiple configs elsewhere, and the system.mail one becomes redundant?

As per @berdir over in #3379794 πŸ“Œ [PP-1] Add symfony mailer transports to DIC Postponed :

the most likely use case for a contrib UI is going to be config entity/plugin based, so even if there is only one, we don't want to read it from raw config, but the config entity API.

.

Mailer transport config is also slightly more involved than the database since transports can be nested. E.g., the Transports transport inspects the X-Transport header of a message and dispatches it to other transports accordingly.

In order to effectively support multi transport configuration in core we'd have to implement a tree-like config structure. A simple key-value model (as with database definitions) wouldn't be sufficient.

πŸ‡¨πŸ‡­Switzerland znerol

I did an extensive research on Symfony messenger today. Starting with the following docs:

Then I went on to examine the contrib project Drupal Symfony Messenger β†’ and its advanced fork Symfony Messenger + Drupal: Realtime Queues and Cron β†’ .

In short, the Symfony messenger component is \Drupal::queue() on steroids. The component allows routing messages of a certain type to different message buses backed by a growing list of message transport types (Database, Redis, etc.). I wouldn't be surprised if this component is going to replace good old queue in some future Drupal release.

I went through the code and the issue queue of the aforementioned contrib modules and found two interesting issues (on the sm fork by @dpi):

Basically MR 69 is refactoring the DSM services structure in a way which is very similar to Symfony Framework Bundle and as a consequence also looks almost exactly the same as to the services structure proposed in this issue. Taking that MR as a starting point it was trivial to make core mailer in this issue work with the sm module. It is enough to add two additional service definitions:

  mailer.messenger.message_handler:
    class: Symfony\Component\Mailer\Messenger\MessageHandler
    arguments:
      - '@mailer.transport'
    tags:
      - { name: messenger.message_handler }
    public: false

  mailer.mailer:
    class: Symfony\Component\Mailer\Mailer
    arguments:
      - '@mailer.transport'
      - '@?messenger.default_bus'
      - '@event_dispatcher'
  Symfony\Component\Mailer\MailerInterface: '@mailer.mailer'

With those services, mails sent via \Drupal::service('mailer.mailer')->send() are automatically routed via Symfony messenger if the sm module is enabled and then reinjected into mailer transports via the MessageHandler.

With all those pieces in place it is much easier to follow the code flow by just stepping through all the components using a debugger.

Email async mailer docs point out that messages sent via the message bus need to be serializable. They especially mention that care should be taken when using doctrine entities in the context property of TemplatedEmail.

The messenger docs point out another detail which needs to be considered when Symfony messenger is used together with the mailer:

The only drawback is that '*' will also apply to the emails sent with the Symfony Mailer (which uses SendEmailMessage when Messenger is available). This could cause issues if your emails are not serializable (e.g. if they include file attachments as PHP resources/streams).

Since Drupal is making it easy to include files from different sources using stream wrappers, I do not think that people need to use resources/streams at all.

Btw. while going through the mailer docs again, I discovered that they introduced a way to add tags and metadata now. Transports and event subscribers may inspect that metadata and act accordingly (#80 is even simpler with that). Also some third-party transports will forward those tags and metadata to cloud messaging providers where they can be used for further processing as well. We can just translate current email ids to tags and be done:

$email->getHeaders()->add(new TagHeader('user_password_reset'));

All of that brings me to the following proposal for a path forward:

  • The service mailer.mailer implementing MailerInterface should be added in this issue (using the original Symfony Mailer class) and it should be the main entry point for the mail delivery part of the new mailer module.
  • Atop of the mail delivery layer, we can add utilities to simplify email building (in other issues).
  • In order to reduce the risk of serialization problems in combination with Symfony messenger, I think we should render HTML messages before they are handed over to the mail delivery layer.
  • Modules implementing hook_mail_alter now who wish to modify the content of messages probably should use the theming layer in the future (preprocess hooks etc).
  • Modules implementing hook_mail_alter now who wish to modify headers or other non-body parts of the message should use event subscribers. They can use tag headers to target individual messages.
πŸ‡¨πŸ‡­Switzerland znerol

Updated the issue summary with information from the original issue πŸ› Correctly implement SessionHandlerInterface Fixed and also linked the CR from over there.

πŸ‡¨πŸ‡­Switzerland znerol

The approach outlined in #81 assumes that transports are stateless. That isn't exactly true and under some corner cases could lead to surprising behavior. Attached are two scripts which I'll be going to use to illustrate a couple of things. The script test-global-transport-factory.php implements the approach from #81. The script test-transport-on-container.php implements the approach taken by Symfony Framework Bundle (and in the current MR). The scripts attempt to send 10 emails in a for-loop.

The following additional tools are recommended in order to reproduce these results:

Connection reuse

Preparation:

  • Inspect the scripts. Make sure that the line define('TEST_DSN', 'smtp://127.0.0.1:1025') is uncommented and all other defines are commented out.
  • Open a terminal and run mailpit (run without any flags mailpit defaults to 1025 for the SMTP port and 8025 for the web interface
  • Open a terminal and run tcpdump. The given bpf filter will log every tcp packet with the SYN flag set which is directed at port 1025 on localhost. Hence, one line will be printed for each tcp connection attempt. People on macOS might need to adapt the interface name (lo0 instead of lo AFAIK):
    sudo tcpdump -q -i lo 'tcp[tcpflags] & (tcp-syn) != 0 and tcp dst port 1025'

Run the test scripts and observe the tcpdump output. Also check the mails in the mailpit inbox.

Result:

  • When test-global-transport-factory.php is run, tcpdump prints 10 lines. The script opened one connection for each message.
  • When test-transport-on-container.php is run, tcpdump prints 1 line. The transport only opens one connection and reuses it for all messages.

Failover behavior

Preparation:

  • Inspect the scripts. Make sure that the line define('TEST_DSN', 'failover(smtp://127.0.0.1:1026 smtp://127.0.0.1:1025)'); is uncommented and all other defines are commented out.
  • Open a terminal and run mailpit (run without any flags mailpit defaults to 1025 for the SMTP port and 8025 for the web interface
  • Open a terminal and run tcpdump. The given bpf filter will log every tcp packet with the SYN flag set which is directed at port 1025 or 1026 on localhost. Hence, one line will be printed for each tcp connection attempt. People on macOS might need to adapt the interface name (lo0 instead of lo AFAIK):
    sudo tcpdump -q -i lo 'tcp[tcpflags] & (tcp-syn) != 0 and tcp dst portrange 1025-1026'
  • Open a terminal and emulate a failing SMTP server on port 1026 (timing out after 5 seconds) using ncat:
    while true; do nc -i 5 -l 1026; done

Run the test scripts and observe the tcpdump output. Measure the time it takes to complete the whole script. Also check the mails in the mailpit inbox.

Result:

  • When test-global-transport-factory.php is run, tcpdump prints 20 lines. The script tries to connect to port 1026 and then falls back to 1025 for each email. The script runs for roughly 50 seconds (10x5sec).
  • When test-transport-on-container.php is run, tcpdump prints 2 lines. First it tries to connect to port 1026, then falls back to 1025 and sends all messages in a row. The script takes roughly 5 seconds to complete.

Conclusion

With this experiment, I'd like to point out that the approach #81 could lead to subtle scaling problems when people start to use the new mailer framework for non-trivial workloads.

Core doesn't send emails in a batch. For password resets, contact messages and update notifications it is irrelevant whether SMTP connections are reused and whether transports are keeping their state or not.

On the other hand, core doesn't need to support multiple transports in the first place. If contrib/custom likes to implement that, then they can already do so on top of the current MR (there is a working multitransport implementation in my examples sandbox β†’ ).

πŸ‡¨πŸ‡­Switzerland znerol

Pinged people from Drupal Symfony messenger integration. They likely will know better than me.

πŸ‡¨πŸ‡­Switzerland znerol

The Symfony framework bundle registers an instance of Symfony\Component\Mailer\Messenger\MessageHandler and injects mailer.transports into its constructor.

πŸ‡¨πŸ‡­Switzerland znerol

The current service structure and entry point respect the requirements of the Symfony messenger component. That one requires a TransportInterface in the container.

If core wants to be compatible with Symfony messenger, then the entry point / service structure I'm proposing here is the way to go.

That is essentially the decision we need to take now.

πŸ‡¨πŸ‡­Switzerland znerol

Load balancing isn't possible with the current mail system in core and it wasn't a goal in the original issue πŸ“Œ Add symfony/mailer into core RTBC . I really think core shouldn't try to implement it. It is an advanced use case and for that kind of things we have contrib.

πŸ‡¨πŸ‡­Switzerland znerol

Added a caveat to the issue summary: The structured DSN doesn't cover a rather exotic use case.

πŸ‡¨πŸ‡­Switzerland znerol

I propose to just link and update the change record β†’ (if this gets in before the 10.2 release). Basically we'd need to update the examples:

In order to configure the DSN, use the following commands:

  • For the default sendmail transport:
    drush config:set system.mail mailer_dsn.schema sendmail
    drush config:set system.mail mailer_dsn.host default
  • For mailhog on localhost:
    drush config:set system.mail mailer_dsn.schema smtp
    drush config:set system.mail mailer_dsn.host localhost
    drush config:set system.mail mailer_dsn.port 1025
  • For authenticated SMTP:
    drush config:set system.mail mailer_dsn.schema smtp
    drush config:set system.mail mailer_dsn.host smtp.example.com
    drush config:set system.mail mailer_dsn.user some-user
    drush config:set system.mail mailer_dsn.password some-pass
πŸ‡¨πŸ‡­Switzerland znerol

Let's focus on the entry point. TransportInterface defines a single method:

    public function send(RawMessage $message, Envelope $envelope = null): ?SentMessage;

It takes a $message of type RawMessage. This signature makes it trivial to send messages which have been serialized before (e.g., because they were queued for asynchronous delivery, cf. messenger component). The RawMessage (and all its subclasses including Email) are data objects. They can be extended when needed. But since there is by design no interface, they cannot be substituted (inheritance trumps composition in this case). From the point in time a message enters the transport subsystem via the send() method, it must be subclass of RawMessage.

Given that, the answers to #79.1 are:

a) The transport id can be stored on the message. The Symfony Transports class uses a custom header for that.
b) This question is related to a). It essentially boils down to this question: How should context be passed from a custom/contrib mail builder to its mail deliver layer through the send() method.

My recommendation is to keep it simple. Add a custom header and store a couple of key value pairs before handing off the message to mailer.transport. Then inspect the metadata in the custom/contrib TransportInterface implementation and act accordingly. If the metadata is missing or if the message isn't of the expected type, then the custom/contrib transport should just forward it to the default transport. That pattern is easy to implement using service decorators. It also works with multiple decorators (coming from different modules). If custom headers don't cut it, then one can subclass Email and use instanceof in the transport implementation.

I'd like to keep the discussion focused on the entry point, thus not going to respond to the rest of #79 at this time.

πŸ‡¨πŸ‡­Switzerland znerol

I added an Architectural aspects section to the issue summary. I hope I captured the important arguments regarding Code location and the Entry point. @Berdir and @AdamPS would you mind checking on that?

I'm planning to add subsections for other subjects which were discussed here in order to help all participants to better understand where we still need to make decisions.

πŸ‡¨πŸ‡­Switzerland znerol

As suggested by @Berdir

what if we have the classes in Drupal\core, but services.yml in a module? the service names wouldn't even have to change then, we could stick with mailer.foo. and the classes can be internal/experimentl

Tried that now and I really like that. Left the test code in the mailer module as well.

πŸ‡¨πŸ‡­Switzerland znerol

Relevant issues are:

πŸ‡¨πŸ‡­Switzerland znerol

@Berdir and @AdamPS what are your thoughts about the mailer_dsn config? Would you rather prefer a key-value mapping instead of a URI like string? I mean, there is hopefully still some time to change that before 10.2 is shipping.

Wouldn't it be nice to deploy something like this in settings.php:

$config['system.mail']['mailer_dsn'] = [
  'scheme' => 'smtp',
  'host' => 'smtp.example.com',
  'port' => 25,
  'user' => 'user@some-domain.org'
  'password' => '^C>6P2EdL.b<?9jBXgOu8*A>'
];

Instead of that:

$config['system.mail']['mailer_dsn'] = 'smtp://user%40some-domain.org:%5EC%3E6P2EdL.b%3C%3F9jBXgOu8%2AA%3E@smtp.example.com:25'

The only thing we'd loose is out-of-the box support for roundrobin and failover transports in core. But with the decoration approach I've outlined above this would be easy to achieve in contrib / custom code.

πŸ‡¨πŸ‡­Switzerland znerol

Made available the Symfony Transport facade in the container (under the same service id as in the Symfony Framework Bundle). Hence, there is now a supported way to call fromString() on a service which has references to all available transport factories.

Seeking feedback on the revised service layout.

(going to update the examples sandbox in a minute).

πŸ‡¨πŸ‡­Switzerland znerol

Please can you clarify where is the example?

In the sandbox linked from #50 and also from the issue summary.

Production build https://api.contrib.social 0.61.6-2-g546bc20