Use structured DSN instead of URI in system.mail mailer_dsn

Love the idea, think we will need a change record for it.

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

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

Load balancing could be supported by providing multiple DSN configs, although it might complicate things a bit.

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

or

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

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.

> It is an advanced use case and for that kind of things we have contrib.
I agree. It's part of my plan for DSM, but it hasn't been implemented yet😃.

I think it's a very good idea, and it's similar to what we already do in DSM.

I see you have matched the names of each parameter to the DSN class, which does make sense. FYI In DSM we used 'pass' (to match the examples in https://symfony.com/doc/current/mailer.html) and 'query' (because they are query parameters).

Rebased so I could run the test-only feature

There was 1 failure:
1) Drupal\Tests\system\Functional\Update\MailDsnSettingsUpdateTest::testSystemPostUpdateMailerDsnSettings
'sendmail://default' does not match expected type "array".
/builds/issue/drupal-3399645/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-3399645/core/modules/system/tests/src/Functional/Update/MailDsnSettingsUpdateTest.php:41
/builds/issue/drupal-3399645/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 1, Assertions: 179, Failures: 1.

If we add to the existing CR think it should be noted with

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

is possible and the problem with load balancing.

rest looks fine.

I do wonder if we should just move to the structure in #9, allowing multiple DSNs but only supporting 'default' in core, similar to the way we allow multiple database definitions but in practice only the 'default' is used except for special cases. 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?

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.

Sending this back to RTBC. I think this is a valid improvement that we should get in before 10.2.0 otherwise we risk having to provide some form of BC.

I was wondering if this did not mean we lost schema validation but it is the opposite, we are gaining a lot of validation here. I wonder if we should add the same regex we added in 📌 Add validation constraint to `type: label` + `type: text`: disallow control characters Fixed , to disallow control characters?

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

I think that can be a follow-up.

Added 📌 Tighten config validation schema of system.mail mailer_dsn Needs work .

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.

Discussed with @catch. We would like to land this in 10.2.0 to avoid adding backward compatibility later, but because we already released beta1 there is a chance some users will now have the old structure in their config.

As we cannot delete post update hooks, we propose making the old post update hook a no-op and overwriting the config with the new structure in a new post update hook. This will cover users upgrading from beta1 or earlier to the next release; we don't expect anyone to have started using this feature yet so we can safely overwrite the config as long as we do it before 10.2.0.

The approach to updates from beta1 to 2 make sense.

I guess we could add something to the release notes about needing to reconfigure it.

Committed and pushed 0b7c2a60a5 to 11.x and 749e1f030f to 10.2.x. Thanks!

Tagging for change record updates, as the examples there need to be updated now.

Also tagging for release notes as we can add a line for beta-to-beta upgraders, just in case anyone started looking at this.

Setting status to NR for the updated change record.

The updated change record looks good to me, thank you @znerol. Added a release note snippet as well.

Automatically closed - issue fixed for 2 weeks with no activity.

znerol → closed merge request !5265