Account created on 8 June 2006, about 18 years ago
#

Merge Requests

More

Recent comments

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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

πŸ‡¨πŸ‡­Switzerland znerol

znerol β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡¨πŸ‡­Switzerland znerol

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

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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.
πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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

πŸ‡¨πŸ‡­Switzerland znerol

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:

πŸ‡¨πŸ‡­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).

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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

πŸ‡¨πŸ‡­Switzerland znerol

znerol β†’ created an issue.

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

Correct screenshot.

πŸ‡¨πŸ‡­Switzerland znerol

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

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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

πŸ‡¨πŸ‡­Switzerland znerol

Oops, did some work in parallel on system.mail.mailer_dsn over in πŸ“Œ Tighten config validation schema of system.mail mailer_dsn Fixed .

πŸ‡¨πŸ‡­Switzerland znerol

Thanks @ptmkenny. Hiding patches.

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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

πŸ‡¨πŸ‡­Switzerland znerol

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
πŸ‡¨πŸ‡­Switzerland znerol

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 +++
πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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.

πŸ‡¨πŸ‡­Switzerland znerol

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

πŸ‡¨πŸ‡­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.

Production build 0.69.0 2024