🇨🇭Switzerland @znerol

@claudiu.cristea I suggest to register a custom service_collector → for the tag: event_subscriber in the container and then just build your own list.

Given the info from #2820364-77: Entity + Field + Property validation constraints are processed in the incorrect order → , I'd be quite surprised if removing the LogicException would be sufficient to support composite validators.

Follow-up: 📌 Remove the restriction from RecursiveContextualValidator that prevents using custom groups. Needs review .

I feel that the proposed changes do have some potential for breakage. Keep in mind that transports are pluggable. And there are some transports in the wild which are using HTTPS to talk directly to an API instead of SMTP.

I did encounter API keys in the past, which leveraged basic auth in weird ways. In one example the whole API key went into the user part, and the password part was supposed to be left blank. See this random bug report on python requests which is referring to that method.

For that reason, I'd prefer if we wouldn't add the NotBlank constraints, unless there are strong reasons for it.

Tracking 📌 Use autoconfigure more in core Active

Rebased 3257293-registered-claims-6.0.x for 6.0.0-beta6.

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

znerol → changed the visibility of the branch 11.x to hidden.

Fresh issue for 6.0.x 🐛 Support reverse-DNS URI scheme Active

znerol → created an issue.

Using this in production for a long time with core and custom entity types.

Copied credits over from 📌 Add validation constraints to system.mail Needs work

I'd suggest to revert 📌 Allow needs_destruction services to run on page cache hits Needs review . The justification for that change isn't that convincing, and the risk assessment expressed in the CR clearly missed the effects on contrib.

Fixed the change record. I hardly can imagine that anybody is overriding this service or calling into addCachedDiscovery() from some weird place. What do I know, though...

Now that 🐛 A revert has cause cspell to fail due to the word yarhar Active is fixed, this shouldn't fail the needs review queue bot anymore.

alexpott → credited znerol → .

Patch in #35 is now obsolete.

Btw, the regression is easily reproducible by just running cspell over a fresh 11.x checkout:

% yarn run spellcheck:core --no-must-find-files --no-progress
./core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php:332:39 - Unknown word (yarhar)
CSpell: Files checked: 15180 (15179 from cache), Issues found: 1 in 1 file.

The cspell thing isn't random. It is reproducible. Any patch/PR which is touching the dictionary will trigger a cspell failure whenever core/scripts/dev/commit-code-check.sh (that's what the needs-review-queue-bot is doing). This is because the commit-code-check.sh conditionally runs a full cspell check:

# Check all files for spelling in one go for better performance.
  if [[ $CSPELL_DICTIONARY_FILE_CHANGED == "1" ]] ; then
    printf "\nRunning spellcheck on *all* files.\n"
    yarn run spellcheck:core --no-must-find-files --no-progress
  else
[...]

The problem is that between the initial commit (#24 from January 1) and the revert of it (#33 May 6) another 📌 Remove 14 foreign words from the dictionary Fixed removed the word yarhar from the global dictionary file. As a result, the revert (which introduced a code chunk containing that word) reintroduced a word into AjaxTest.php (see needs-review-bot output from #34).

Running spellcheck on *all* files.
./core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php:332:39 - Unknown word (yarhar)

CSpell: failed

The patch in #35 is fixing that regression.

Added a patch, I guess we'd need something like that.

It looks like the revert (i.e., commit 966fd63a6c7fd5ae9cd61222717eac2644efc872) is causing the needs-review-queue-bot to reject all issues on needs review. The problem seems to be the string yarhar. Recent example:

• https://www.drupal.org/project/drupal/issues/3401255#comment-15602961 📌 Tighten config validation schema of system.mail mailer_dsn Fixed
• https://www.drupal.org/files/issues/2024-05-20/3401255-nr-bot_0.txt →

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

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.