🇨🇭Switzerland @znerol

In an earlier revision → of the issue summary, there was the following bullet point:

• Decide on an approach on how mails are going to be built.

Let's turn that bullet point into an issue.

Message::getPreparedHeaders() has code that overlaps/conflicts with this issue (I'm looking at v6.4.17). It will set from based on sender so potentially we don't need setDefaultFrom().

Interesting idea. I tested that. The very first test fails at line 55 (last assertion in the following snipped).

    $originalEmail = $sentMessage->getOriginalMessage();
    assert($originalEmail instanceof Email);
    $actualFrom = $originalEmail->getFrom();
    $this->assertEquals([$expectedAddress], $actualFrom);

The test expects that $originalEmail->getFrom() contains the site address. This isn't the case, because Message::getPreparedHeaders() does only derive, but not save the from header.

However, imagine a message subscriber which logs messages which failed. This might want to log the from header as well.

And it will set sender based on from, which will undo the work of removeRedundantSender().

That is not true. removeRedundantSender() only removes sender if there are multiple from addresses. Message::getPreparedHeaders() only adds sender when there is none and when there are multiple from addresses.

Regarding the process, nothing blocks this. Even if the implementation should change at some point, its good to have the tests in place. And I guess it will take some time and energy to review them as well.

Please let's keep this issue a low-noise place. I'd prefer if people could follow here in order to just get updates about the progress, nothing more, nothing less. See the 🌱 Single Directory Components module roadmap: the path to beta and stable Active for how this kind of issue has been used in other core initiatives.

Thanks for the review. Regarding #11, we should stick with AbstractTransport, otherwise transport events will not fire. I've added methods for both cases (getEmails() in order to assertions on the original Email and getCapturedMessages() for assertions on SentMessage).

znerol → created an issue.

znerol → created an issue.

adamps → credited znerol → .

With !4 existing deployments will need to change their config and append /auth to the configured url. Is that acceptable?

Regarding the standard. There are probably a gazillion of ways on how keycloak can be deployed. The official keycloak container (docs) defaults to no path prefix at all. This is actually how I found the issue (over in 📌 Add integration tests Active ). Maybe if you run keycloak using a preexisting helm chart or ansible playbook, things might be different.

Nothing ready yet. Just testing whether gitlab ci on drupal.org is capable of running keycloak in concert with phpunit.

znerol → created an issue.

znerol → created an issue.

znerol → created an issue.

Thanks! Added an update hook.

Just the very basics. Interestingly phpstan detects the D11 issue ( 🐛 Drupal 11 compatibility Active ).

znerol → created an issue.

znerol → created an issue.

Added both libraries in the PR (sodium and argon2). Its the way debian php is built.

It would be possible to minimize dependencies by not adding sodium at all and stick with argon2 library. From PHP 8.4, even that one is not necessary anymore. From that version onwards it is possible to use OpenSSL Argon2 implementation. Question here: Is it preferable to remain close to what distros are shipping or is it preferable to minimize dependencies.

Test command and example result for images with argon2 support:

% podman run -it --rm docker.io/drupalci/php-8.5-ubuntu-apache:dev php -r 'echo(password_hash("correct horse battery staple", PASSWORD_ARGON2ID));'
$argon2id$v=19$m=65536,t=4,p=1$MzFSb1Q0MVg1NDBJOHQ4Nw$SYS269U5LJL2TEBG9t/glMS83kYMKB9mU9AJ4pnyO24

Postponing on #3530235

znerol → created an issue.

znerol → created an issue.

Bug fixes are cherry-picked by committers to active branches, see the core Backport Policy → .

This has its origins in 🌱 [Meta, Plan] Pitch-Burgh: Policy based access in core Active . I guess the interface has been renamed during the development process. But @core.services.yml@ has been forgotten.

% git grep AccessPolicyChainInterface
core/core.services.yml:  Drupal\Core\Session\AccessPolicyChainInterface: '@access_policy_processor'

% git grep AccessPolicyProcessorInterface
core/lib/Drupal/Core/Session/AccessPolicyProcessor.php:class AccessPolicyProcessor implements AccessPolicyProcessorInterface {
core/lib/Drupal/Core/Session/AccessPolicyProcessorInterface.php:interface AccessPolicyProcessorInterface {
[...]

Nightwatch test fails are random ones 🌱 [meta] Known intermittent, random, and environment-specific test failures Active .

znerol → created an issue.

znerol → created an issue.

Thanks for taking this up. Adding a data provider is useful in this case I think. It needs some improvements in order to improve readability I think.

Something like this could go into a response subscriber:

$fids = $session->get('anonymous_allowed_file_ids', []);
if (!empty($fids)) {
  $fids_unref = [];
  $files = $this->fileStorage->loadMultiple($fids);
  foreach ($files as $file) {
    $references = $this->fileUsage->listUsage($file);
    if (empty($references)) {
      $fids_unref[] = $file->id();
    }
  }
  if (empty($fids_unref)) {
    $session->remove('anonymous_allowed_file_ids');
  }
  else {
    $session->set('anonymous_allowed_file_ids', $fids_unref);
  }
}

You can try this in a custom module as a quick fix until somebody comes up with a clever approach.

git log -S anonymous_allowed_file_ids indicates that this mechanism has its origins in SA-CORE-2017-003 → (Files uploaded by anonymous users into a private file system can be accessed by other anonymous users).

https://git.drupalcode.org/project/drupal/-/commit/c732355412b84a6f7079d...

The code comment at the top of the relevant hunk reads:

        // This case handles new nodes, or detached files. The user who uploaded
        // the file can always access if it's not yet used.

This indicates that this branch in anonymous_allowed_file_ids">FileAccessControlHandler::checkAccess() is only reached as long as the parent entity (e.g. media) is not saved yet.

I think that the file id should be cleared from the anonymous_allowed_file_ids session value as soon as the temporary upload file is moved to permanent storage. If anonymous_allowed_file_ids becomes empty, then it needs to be removed from the session. And as soon as the whole session is empty, it will be destroyed automatically.

Thanks @jpoesen for being persistent in this matter. And thanks for being polite and constructive.

znerol → created an issue. See original summary → .

Thats good news! Thanks.

I assume retry_period from the changelog is not relevant to us?

No. Its injected into RoundRobinTransport if the DSN is parsed from a string. Since we are using ✨ Use structured DSN instead of URI in system.mail mailer_dsn Fixed , this isn't in the code path.

Follow-up: 📌 Extract testUriHost from UuidValidatorTest into its own class Active .

znerol → created an issue.

znerol → created an issue.

Added a CR draft.

Thanks. Opened 🌱 Single Directory Components module roadmap: the path to beta and stable Active (cowardly copied over from 🌱 Single Directory Components module roadmap: the path to beta and stable Active )

znerol → created an issue.

Thanks, I like all of the suggestions.

I'm also wondering if the mail theme classes shouldn't be in the `Drupal\Core\Mail` namespace instead of `Drupal\Core\MailTheme`?

My plan was to deprecated everything in Drupal\Core\Mail eventually. 📌 [PP-1] Add symfony mailer transports to DIC Postponed introduces Drupal\Core\Mailer namespace as a replacement.

Thanks @larowlan for the review.

the main concern from an FM POV is the number of transports added that may be redundant.

.

The MR adds one transport to the container (i.e., one instance of Symfony\Component\Mailer\Transport\TransportInterface). But in order to be able to instantiate that transport, the presence of N transport factories is necessary. Note that AutowireIterator iterates through the list of service definitions tagged with mailer.transport_factory and instantiates one-by-one until it finds one which is capable of creating the desired transport (defined by the DSN scheme).

Also, it turned out that the transport factories can be market non-public, yay! The only two public services added by the MR are now Symfony\Component\Mailer\Transport\TransportInterface and Symfony\Component\Mailer\MailerInterface.

I wasn't happy how GitLab makes it difficult to access the recent failures data through the web UI. So, I took a shot at extracting these numbers from the API.

• flaky_tests_data is a repository with the results. A pipeline is rebuilding it daily. The data is also accessible via GitLab Pages (e.g., latest.json).
• flaky_tests_scraper code for the scraper.
• flaky_tests_dashboard a small PoC which lists a summary of failing tests in a given dataset.

There is still lots in the data which isn't shown in the PoC dashboard. Please use the issue tracker of the linked projects or hit me up on slack if you have suggestions on how to improve it.

Latest data is accessible via https://project.pages.drupalcode.org/flaky_tests_data/drupal/latest.json

znerol → created an issue.

znerol → created an issue.

znerol → created an issue.

@naidim this is just declaring a library dependency, not a module dependency. You still can use the Field Group module in production without enabling Field UI.

Repeat test MR is passing. However, there is a fail in EntityReferenceWidgetTest::testWidgetPreview(). Is that an instance of a known random test fail?

https://git.drupalcode.org/issue/drupal-3514699/-/pipelines/476020/test_...

According to #65 this is fixed in PHP. Closing.

I agree with #6, issue #2921123: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+ → contains valuable research leading to the fix. However, I think we can trust people to use git blame to actually find that research (via this issue). I removed the @see reference.

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

Rerolled #40 and migrated to PR, still needs work for #48/#49.

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

Tests are passing now.

Debugging FunctionalJavascript tests is f*** fun. I was trying numerous things to resolve a mysterious race condition yesterday. Looking at this with a fresh mind today, I discovered that saving the layout resulted in the following stack trace:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for core.entity_view_display.commerce_product.default.default with the following errors: core.entity_view_display.commerce_product.default.default:third_party_settings.layout_builder.sections.0.components.25d9bddb-abbb-451a-8ab8-1aaa70ee866f.configuration.formatter.settings.redirect missing schema in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 98 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php).

Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}() (Line: 206)
Symfony\Component\EventDispatcher\EventDispatcher->callListeners() (Line: 56)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch() (Line: 230)
Drupal\Core\Config\Config->save() (Line: 260)
Drupal\Core\Config\Entity\ConfigEntityStorage->doSave() (Line: 486)
Drupal\Core\Entity\EntityStorageBase->save() (Line: 239)
Drupal\Core\Config\Entity\ConfigEntityStorage->save() (Line: 354)
Drupal\Core\Entity\EntityBase->save() (Line: 617)
Drupal\Core\Config\Entity\ConfigEntityBase->save() (Line: 170)
Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay->save() (Line: 311)
Drupal\layout_builder\Plugin\SectionStorage\DefaultsSectionStorage->save() (Line: 159)
Drupal\layout_builder\Form\DefaultsEntityForm->save()
call_user_func_array() (Line: 105)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 43)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 589)
Drupal\Core\Form\FormBuilder->processForm() (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult() (Line: 39)
Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 37)
Drupal\Core\Test\StackMiddleware\TestWaitTerminateMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 709)
Drupal\Core\DrupalKernel->handle() (Line: 19)
require('[...]/index.php') (Line: 48)

To the future me: I got hold of that trace by the following lines:

    $content = $this->getSession()->getPage()->getContent();
    file_put_contents('/tmp/commerce-test-fail.html', $content);

Thanks @alexpot for the hint.

I've been trying to reproduce the test fails locally (#devdays). I am able to repro some fails sometimes, bot not consistently. Even worse: As soon as I attempt to set breakpoints at the affected locations, tests will pass.

It very much looks like ProductLayoutBuilderIntegrationTest is subject to race conditions - however, I'm not sure whether this issue makes them surface more often.

Added a CR.

MR 409 is ready for review.

That's for 11.3, not 11.2

Right, then there is no need to delay the deprecation, I guess.

znerol → created an issue.

znerol → created an issue.

In both SessionTestController and LegacySessionController, the method documentation often declared a completely wrong return type:

  * @return string
  *   A notification message.

Since this MR touches a couple of those methods, I fixed them all.

I agree that deprecating direct access to the $_SESSION is not disruptive. However, the next step will be to remove support for custom session keys completely. I.e., SessionManager would stop considering custom keys in $_SESSION when deciding whether to start/save a session automatically.

If contrib and custom code continue to access custom keys, this might or might not work depending on other circumstances (i.e., whether or not some unrelated part of the system is using the session as well). But problems will be hard to diagnose because custom keys are silently ignored when the deprecation and support for custom keys are dropped.

That was the reason I think a longer deprecation period could be worthwhile in this case.

Added a legacy test and a CR.

Needs tests for the new deprecation. Also needs a CR for sure.

It is an issue and it is reproducible reliably with the steps in the IS. But probably not easy for test automation (because its a race condition).

znerol → created an issue.

Closing this as outdated.

If you host a single site on a single deployment: Specify options.uri in drush.yml and place that file in one of the following locations sites/all/drush, WEBROOT/drush, or PROJECTROOT/drush. This will be used as the fallback if nothing else is specified (see drush configuration docs - directories and discovery.

Look into site aliases if you want to do something more complicated. E.g., one site serving multiple domains (example.com, example.org, example.net) or staged deployment (dev, test, staging, production).

Do not attempt to use environment variables unless you are operating in a containerized environment.

Also please study drush configuration docs, especially the Global options section. If the options.uri is configured, any drush invocation will use that as the default value for --uri. This is useful for sites not running on any of the specialized hosting packages (or ddev).

I'd like to avoid the pitfall of wishful/magic thinking here. If drush is supposed to use this feature, then there should be a PoC for drush which demonstrates how this is supposed to be integrated.

Also if all of the projects mentioned in the issue summary and the comments which are supposed to profit from that feature need to be modified, how do we ensure that this is implemented in a consistent way across the board?

I'm confused. Is this setting supposed to have any influence on url generation? Like, e.g., on absolute url in emails sent during a cron run? If yes, how would that work with the current implementation? If no, why add it in the first place?

how to entirely get rid of preprocess via SDCs yet

I'm more likely to implement a twig extension instead of a preprocess function nowadays. Maybe it would be enough to just improve the DX around twig extensions. E.g., attributes:

#[TwigFilter('without')]
public function withoutFilter($element, ...$args) {
// [...]
}

#[TwigFunction('url', options: ['is_safe_callback' => Foo::isUrlGenerationSafe])]
public function getUrl($name, $parameters = [], $options = []) {
// [...]
}

Nice approach!