🇨🇭Switzerland @znerol

Added tests for the new UriHost constraint. Note that filter_var($value, \FILTER_VALIDATE_DOMAIN, \FILTER_FLAG_HOSTNAME) accepts every valid IPv4, thus removed the explicit call to filter_var($value, \FILTER_VALIDATE_IP, \FILTER_FLAG_IPV4).

Gist of a slack conversation with @penyaskito:

@znerol saw that, but still think hostname should re-use symfony validator instead of just validating no control characters. The stricter the better. And even if we don't, I'm pretty sure we will need to reuse the symfony constraints at some point.

Turned out that the Hostname validator isn't quite enough, since the host parameter also should accept IP addresses. I've investigated whether it would be possible to implement this using the AtLeastOneOf and a combination of Hostname and Ip. But that doesn't allow for IPv6 literals (enclosed in brackets).

I guess it would be best to just introduce an UriHost constraint, which implements RFC 3986 section 3.2.2.

Thank you. Posted a link to the repo on the project page.

znerol → created an issue.

znerol → created an issue.

I worked through the Integration with Media module → documentation guide, and that works without issues in Drupal 10.2.

Please try to follow the docs exactly, maybe something isn't configured properly.

Correct screenshot.

@Berdir or @Primsi: would you mind taking a look at the MR?

PaymentOrderUpdater has its origins in #3011667: Saving the order before its payment in PaymentGateway::onReturn() can cause data loss → . It has been converted into a destructable service in #3085813: Consider moving from Kernel::TERMINATE event to DestructableInterface implementation → . A similar issue has been reported in #3124179: Render context is empty after IPN payment → . Another backtrace referring to the same code location here #3158347: PaymentOrderUpdater destruct called after migration → (related to migrate, though).

My hunch is that something renders a link, an email or some other piece of markup in an event subscriber. That results in a cache metadata leak while the webhook is processing. In order to prevent cache metadata leaking, we might want to wrap the webhook logic into a render context and throw away the results.

znerol → made their first commit to this issue’s fork.

The phpstan job in GitLab CI isn't reporting new issues when SDK 4 is installed. And people still can continue using SDK 3. Thus, merging that in order to simplify real world testing.

znerol → created an issue.

znerol → created an issue.

I added the FullyValidatable constraint to the @system.mail@ config. But honestly, I'm not quite sure on which level this should be added (config level or core datatype level, or even/additionally in @mailer_dsn.options.x@).

BramDriesen → credited znerol → .

borisson_ → credited znerol → .

Oops, did some work in parallel on system.mail.mailer_dsn over in 📌 Tighten config validation schema of system.mail mailer_dsn Postponed .

Switched parent to #2952037

Added tests.

Thanks @ptmkenny. Hiding patches.

I think it would be nice to have this configurable in a similar manner as twig debugging (see ✨ Make it easier for theme builders to enable Twig debugging and disable render cache Fixed and 📌 Move twig_debug and other development toggles into raw key/value Active ).

Note that with 📌 [PP-1] Add symfony mailer transports to DIC Postponed , mailer related services will be registered in the container. Since $logger is supposed to be injected into transports (via transport factories), I think that a compiler pass based approach is sensible here.

Chase head and update 📌 [PP-1] Add a way to capture mails sent through the mailer.transport service during tests Postponed .

Indeed. The log messages produced by the symfony mailer component are useless for most production setups.

% grep -i 'log[^- ]*->' -r vendor/symfony/mailer
vendor/symfony/mailer/DataCollector/MessageDataCollector.php:        $this->events = $logger->getEvents();
vendor/symfony/mailer/Transport/AbstractTransport.php:            $this->logger->debug(sprintf('Email transport "%s" sleeps for %.2f seconds', __CLASS__, $sleep));
vendor/symfony/mailer/Transport/SendmailTransport.php:        $this->getLogger()->debug(sprintf('Email transport "%s" starting', __CLASS__));
vendor/symfony/mailer/Transport/SendmailTransport.php:        $this->getLogger()->debug(sprintf('Email transport "%s" stopped', __CLASS__));
vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:                $this->getLogger()->debug(sprintf('Email transport "%s" stopped', __CLASS__));
vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:        $this->getLogger()->debug(sprintf('Email transport "%s" starting', __CLASS__));
vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:        $this->getLogger()->debug(sprintf('Email transport "%s" started', __CLASS__));
vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:        $this->getLogger()->debug(sprintf('Email transport "%s" stopping', __CLASS__));
vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:            $this->getLogger()->debug(sprintf('Email transport "%s" stopped', __CLASS__));
vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php:            $this->getLogger()->debug(sprintf('Email transport "%s" sleeps for %d seconds after stopping', __CLASS__, $sleep));

Also, symfony docs recommend to use event subscribers and the getDebug() method in order to gather useful information.

Serving responses with Cache-Control: no-store has been the source of wicked UX issues in the past. This needs careful manual evaluation of multiple scenarios across popular browsers. Some scenarios are described in this rather old atlassion blog post.

One scenario based on Drupal core alone could use the contact form together with a misconfigured e-mail transport. This accurately simulates a situation on a production site where a mail server isn't reachable for a short time. In this case an error message is displayed after the submit button has been pressed. My reaction as a user of this site would be to press the back button in order to get back to the text I've just written, either to save it to a file or to retry the form submission. On a site with Cache-Control: no-store, I fear that the browser will render an empty contact form and that all text is gone.

My hunch is that the Cache-Control: no-store response header should be used on specific routes, i.e., on pages which display sensitive / confidential data. Examples of this type of information would be social security number, personal health information or credit-card data. Forms which accept and pages which display this kind of data probably should supply their own Cache-Control: no-store header. And security audits will rightfully flag such pages if that isn't the case.

Also please note that results of any automated reporting tools (including security scanners) need to be interpreted by people with knowledge in that particular field. Comment #5 gives no details about the type of application, the scope of the audit and the reason for the suggested change.

znerol → created an issue.

The current MR in 📌 Remove calls to Request::hasSession() Active removes PrivateTempStore::startSession() entirely.

I suggest we close this issue here as a won't fix.

Patch #8 is working for me on an existing site with existing config. Haven't tested with a fresh one though.

With the current MR we are down to zero references to the $_SESSION superglobal (except where it is required in SessionManager and SessionTestController).

% git grep -l '$_SESSION'
core/lib/Drupal/Core/Session/SessionManager.php
core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php

Good question.

The following command lists changes in production code. The one in LanguageNegotiationSession could be pushed to 📌 Remove calls to Request::hasSession() Active . But since the comment needs to be updated to point to the correct issue anyway, it doesn't hurt to fix it right here I think.

% COLUMNS=999 git diff `git merge-base HEAD 11.x` --stat | grep -vi test
 core/includes/batch.inc                                                             | 16 ++++++++++------
 core/includes/form.inc                                                              | 14 +++++++++-----
 core/lib/Drupal/Core/Form/FormBuilder.php                                           |  8 +++++---
 core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php |  4 +---

The rest of the changes are in tests, those are mostly necessary due to 📌 Add the session to the request in KernelTestBase, BrowserTestBase, and drush Fixed . Plus the CSRF fix in BigPipe.

% COLUMNS=999 git diff f8a2ccac36a8402ec404e4b7c421e073fcf0d052 --stat | grep -i test
 core/modules/big_pipe/tests/src/Functional/BigPipeTest.php                          | 15 +++++++++++++++
 core/modules/user/tests/src/Kernel/UserAccountFormPasswordResetTest.php             |  9 +--------
 core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php                               |  4 ++++
 core/tests/Drupal/Tests/Core/Form/FormTestBase.php                                  |  3 +++

Let's try to keep the order of test cases. Instead, ensure that a session is started before storing the CSRF value in the session bag on the test runner.

Should login and logout be events instead? Then each subsystem can declare event subscribers and react as they see fit, and each part would be replaceable individually if someone wanted to switch something out?

I was thinking the same.

There is no patch required for image replace for Drupal 9 or 10. Please follow the respective page in the documentation guide → instead.

Reroll succeeded without conflicts.

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

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.

Squashed the early commits up to #3379794-37: Add symfony mailer transports to Dependency Injection Container → in order to minimize conflicts in future rebases.

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.

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.

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

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.

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).

Added @todo

Opened the follow-up 📌 Remove calls to Request::hasSession() Active .

znerol → created an issue.

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

Quick status update:

• Drupal Symfony Mailer Lite → added support for Symfony Messenger in a way which is compatible with the approach proposed here (see ✨ Add Symfony Messenger support for async messsages (emails as queues) Needs review ).
• Core added 📌 Add Psr\EventDispatcher\EventDispatcherInterface alias to core services Needs review , so we can autowire: true for the mailer service now.

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

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)

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

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.

znerol → created an issue.

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.

MR should be up-to-date now.

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.

Fine to self RTBC

Thanks @larowlan. Done.

znerol → created an issue.

znerol → created an issue.

Thanks a lot @dpi

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.

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).

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.

CR added.

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.

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

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.

Setting status to NR for the updated change record.

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.

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.

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.

znerol → created an issue.

Adding this to the meta issue #3050720

Added 📌 Tighten config validation schema of system.mail mailer_dsn Postponed .

Postponed on ✨ Use structured DSN instead of URI in system.mail mailer_dsn Fixed .

znerol → created an issue.

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

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