Account created on 1 April 2020, over 5 years ago
  • Developer at Liip 
#

Merge Requests

More

Recent comments

🇨🇭Switzerland tcrawford

Currently blocked by Issue #3531714 - Drupal 11.2.0 incompatibility (sebastian/diff incompatiblity due to config_patch).

🇨🇭Switzerland tcrawford

I have updated the patch file for the alternative approach to handling system files.

🇨🇭Switzerland tcrawford

It turns out that it is better to process system files with the base scheme instead of the internal scheme. Otherwise URLs will be ouput as /system/files?file=...... I will fix this and push to the MR shortly and update the patch.

🇨🇭Switzerland tcrawford

Added a test for the fix for system files.
Here is the patch to the current latest commit on MR 12: pathologic-language-prefix-2418369-84.diff

🇨🇭Switzerland tcrawford

I tested this and it worked for me in the case of linking from a German node with an anchor to a node in another language (e.g. French link

However, for the case of system files (e.g. some.pdf the patch results in the URL being mangled. I debugged this and found the cause.

System private files are now processed with the internal: scheme, whereas without the patch they are processed with the base: scheme. This (internal:) seems semantically correct, because they are routed. However, this has a nasty side-effect.

The _pathologic_replace callback passes options to fromUri(), with options that contain an empty query param.
$url = Url::fromUri($path, $url_params['options'])->toString();

fromUri() then calls from static::fromInternalUri(). Here the options with an empty query param array are passed to the URL.

This overrides the file query param that is added by the PathProcessorFiles (inbound path processor), resulting in the files query param being lost and the URL being rendered as
$url->setOptions($options + $url->getOptions());

We likely need to treat system files as base: (does not seem semantically correct, but works) or unset the the query key from the options before calling Url:fromUri. I tested both approaches and they work. I will push this change to the MR and try to add a test for this case.

🇨🇭Switzerland tcrawford

I suggest to work only on the MR (and provide patches on this, as kensae has done) rather than also providing new patches. It is difficult to compare #44 against #42 without creating an interdiff.

I installed patch #42 (based on MR!8) and I see type errors in the console. A colleague will test the functionality further, and we will provide feedback when available.

gtag.js?svsmmn:22 Uncaught TypeError: Cannot read properties of null (reading 'length')
at gtag.js?svsmmn:22:20

🇨🇭Switzerland tcrawford

I am working on a project where this issue has suddenly surfaced after a Drupal 11 update. I am also not sure why we have also redirect_fallback_action of NULL on most node types. To resolve, I have just opened the node type config and resaved.

🇨🇭Switzerland tcrawford

Thanks for the feedback.
In best case, a redirect to the login page of another Drupal domain will result in the destination parameter being lost.
The domain access (and not just the domain source) module might be required to first trigger the 403.
That said, we also have other custom access and tac lite ( https://www.drupal.org/project/tac_lite ) installed on the project, and I am not 100% sure they are also not required to reproduce (although looking at the stack trace suggests not).
I would have to test the scenario on a clean install, but have not yet had the possibility to do that.

🇨🇭Switzerland tcrawford

We have the same symptom, but a slightly different cause and therefore MR87 does not work for us.
In our case, we have a custom form mode and $form['account']['pass'] is not set at all. Therefore, the check falsely identifies the password as being mutable.

$password_mutable = $form['account']['pass']['#access'] ?? TRUE;

I would suggest the check could be changed to the following, where we check if the pass field is even present and if so check the access to that. I would also reverse the order of the and as it is less expensive that way.

  $password_mutable = (isset($form['account']['pass']) && ($form['account']['pass']['#access'] ?? TRUE));

  // Load form if relevant.
  if ($password_mutable && \Drupal::service('password_policy.validation_manager')->tableShouldBeVisible()) {

The check also needs to be added to check if validation should run (otherwise we just don't show the table rows, but validation will still catch us). That possibly should be though in the PasswordPolicyValidationManager.

if ($password_mutable && \Drupal::service('password_policy.validation_manager')->validationShouldRun()) {
    $form['#validate'][] = '_password_policy_user_profile_form_validate';
    $form['#after_build'][] = '_password_policy_user_profile_form_after_build';
  }
🇨🇭Switzerland tcrawford

I have created an MR. We are testing this (by using the issue fork as a composer repository, as drupal lenient can't be used due to the composer dependency). Therefore, I am moving this on to 'needs review'.

   "commerce_payrexx_integration": {
            "type": "package",
            "package": {
                "name": "drupal/commerce_payrexx_integration",
                "version": "2.0.1",
                "type": "drupal-module",
                "source": {
                    "url": "https://git.drupalcode.org/issue/commerce_payrexx_integration-3513451.git",
                    "type": "git",
                    "reference": "685b6c29d9946151be20e16355e674b3de2cde70"
                }
            }
        },
🇨🇭Switzerland tcrawford

We had the same issue on a project. The above hint was very helpful. Thank you.
I wonder if the menu item recursion should not be checked during a cache clear to prevent such configurations getting into production.

🇨🇭Switzerland tcrawford

I am re-assigning this as there are some minor issues with the view.

🇨🇭Switzerland tcrawford

I have added the view for commerce product variations, adapted the derivative plugin and updated the view access tests. I am therefore assigning this one to 'needs review'. Thanks in advance.

🇨🇭Switzerland tcrawford

@Jonathan1055: Sorry. I missed your prior message. I now have tests passing now. I just need to add the view and integrate this and consider whether to do the rules integration.

🇨🇭Switzerland tcrawford

Thanks for the feedback on where to include the plugin.

But is there any benefit in putting it within a submodule of Scheduler which only gets enabled when required?

I would tend to suggest to leave it in the main module as it is consistent with the commerce product plugin.

Is this why you added 'language', 'locale' and 'config' to SchedulerBrowserTestBase here ?

Yes. I have removed these though as they were not required.

🇨🇭Switzerland tcrawford

Hi @jonathan1055. Thanks for the feedback. I had not, but I just read the documentation.

If you would prefer the plugin for product variations to be stored in a third party module or is it a commonly used enough entity to be included here (perhaps due to its relation to the commerce product entity that it included)?

Unfortunately, I had some issues running these tests locally (the runner is complaining about missing dependencies that don't seem to be an issue on the server). I reached out to pfrenssen, who is a colleague, for some assistance and so if I can get the tests running locally I will save some resources on the server and be a bit faster.

Great idea, I will comment out data types (now that the prior tests are passing) to get feedback on the new entity type only.

🇨🇭Switzerland tcrawford

tcrawford made their first commit to this issue’s fork.

🇨🇭Switzerland tcrawford

The automated patch that was merged in MR 3 unfortunately missed the composer dependency. I am fixing this now.

🇨🇭Switzerland tcrawford

I created MR 16 (against 2.x) that also contains the changes to the info file. This needs a review.

🇨🇭Switzerland tcrawford

This issue was created against 2.x and MR 15 does not apply against 2.x (only 1.x). MR 12 does not adapt the core version requirement. Therefore, I am placing this issue back to needs work. I suggest to create a separate issue for Drupal 11 compatibility for 1.x.

🇨🇭Switzerland tcrawford

Thanks Milan. Much appreciated. I confirm the patch worked. We probably need to consider a sustainable fix here, such as making this optional.

🇨🇭Switzerland tcrawford

I bumped in to the issue.
In our case we have a form display mode that does not have the name field present, which is a valid use case.
We have the MR from https://www.drupal.org/project/password_policy/issues/2451159 Password policy doesn't work when updating the user Needs work installed as a patch, but the issue can be reproduced without this and the patch resolves the issue regardless.
The name is actually already persisted on the entity.

Error:

```

Warning: Undefined array key "account" in /app/docroot/modules/contrib/password_policy/password_policy.module on line 243

Warning: Trying to access array offset on value of type null in /app/docroot/modules/contrib/password_policy/password_policy.module on line 243

Warning: Trying to access array offset on value of type null in /app/docroot/modules/contrib/password_policy/password_policy.module on line 243

Warning: Undefined array key "#default_value" in /app/docroot/modules/contrib/password_policy/password_policy.module on line 243
The website encountered an unexpected error. Try again later.

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'name' cannot be null: INSERT INTO "users_field_data" ("uid", "langcode", "preferred_langcode", "preferred_admin_langcode", "name", "pass", "mail", "timezone", "status", "created", "changed", "access", "login", "init", "default_langcode") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14); Array ( [:db_insert_placeholder_0] => 52200 [:db_insert_placeholder_1] => de [:db_insert_placeholder_2] => de [:db_insert_placeholder_3] => fr [:db_insert_placeholder_4] => [:db_insert_placeholder_5] => $2y$10$O0rUbJ7ueExLy7vEmBfVp.oOv5Vzl1xnQM52SOyp8rsy2JK5RcD0W [:db_insert_placeholder_6] => sanitized@example.com [:db_insert_placeholder_7] => Europe/Zurich [:db_insert_placeholder_8] => 1 [:db_insert_placeholder_9] => 1737379851 [:db_insert_placeholder_10] => 1737473718 [:db_insert_placeholder_11] => 1737380061 [:db_insert_placeholder_12] => 1737380061 [:db_insert_placeholder_13] => sanitized@example.com [:db_insert_placeholder_14] => 1 ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 817 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
Drupal\Core\Database\StatementWrapperIterator->execute(Array, Array) (Line: 44)
Drupal\mysql\Driver\Database\mysql\Insert->execute() (Line: 1022)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->saveToSharedTables(Object) (Line: 935)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->doSaveFieldItems(Object, Array) (Line: 33)
Drupal\user\UserStorage->doSaveFieldItems(Object) (Line: 718)
Drupal\Core\Entity\ContentEntityStorageBase->doSave('52200', Object) (Line: 486)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 806)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object) (Line: 354)
Drupal\Core\Entity\EntityBase->save() (Line: 293)
Drupal\Core\Entity\EntityForm->save(Array, Object)
call_user_func_array(Array, Array) (Line: 129)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 67)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 597)
Drupal\Core\Form\FormBuilder->processForm('user_pharmacies_form', Array, Object) (Line: 326)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 68)
Drupal\simple_oauth\HttpMiddleware\BasicAuthSwap->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 53)
Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 742)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
```

Cause:
Function _password_policy_user_profile_form_validate(&$form, FormStateInterface $form_state) sets the name to an empty value if not found in the form.

Proposed solution:
As above. I am moving this back to needs review. Please consider again the prior proposed solution. https://www.drupal.org/files/issues/2023-01-12/3332884-integrity-constra...

// set username if form_state name value is empty
if (empty($form_state->getValue('name'))) {
$user->setUsername($form_state->getValue('name', $user->name->value));
}

🇨🇭Switzerland tcrawford

I have created a separate module with an event subscriber to set the checkout language on the order. The rational for a separate module is that it can be activated or de-activated, thereby avoiding the need for configuration. I use both the add to cart and checkout completion events (see the prior comment). I can create the module if you agree, otherwise, I can update the MR for core (with tests) if agreed.

<?php

declare(strict_types=1);

namespace Drupal\commerce_checkout_language\EventSubscriber;

use Drupal\commerce_cart\Event\CartEntityAddEvent;
use Drupal\commerce_cart\Event\CartEvents;
use Drupal\commerce_checkout\Event\CheckoutEvents;
use Drupal\commerce_order\Entity\OrderInterface;
use Drupal\commerce_order\Event\OrderEvent;
use Drupal\Core\Language\LanguageManagerInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Sets the checkout language on the order.
 */
final class CommerceOrderLanguageSubscriber implements EventSubscriberInterface {

  /**
   * Constructs a CommerceCheckoutLanguageSubscriber object.
   */
  public function __construct(
    private readonly LanguageManagerInterface $languageManager,
  ) {}

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents(): array {
    return [
      CheckoutEvents::COMPLETION => ['onCheckoutCompletion'],
      CartEvents::CART_ENTITY_ADD => ['onCartEntityAdd'],
    ];
  }

  /**
   * Respond to the checkout completion event.
   *
   * @param \Drupal\commerce_order\Event\OrderEvent $event
   *   The order event.
   */
  public function onCheckoutCompletion(OrderEvent $event): void {
    // The checkout completion event requires the user to return to the checkout
    // after any offsite gateway.
    $this->setOrderLanguage($event->getOrder());
  }

  /**
   * Respond to the cart entity add event.
   *
   * @param \Drupal\commerce_cart\Event\CartEntityAddEvent $event
   *   The cart entity add event.
   */
  public function onCartEntityAdd(CartEntityAddEvent $event): void {
    // The cart entity add event is fired when a purchasable entity is added to
    // the cart. This is used as a default, which may be overridden by the
    // checkout completion event. We do this to ensure that the order language
    // is set even if the order is completed in the IPN (and the user does not
    // return to the checkout).
    $this->setOrderLanguage($event->getCart());
  }

  /**
   * Sets the order language to the current language.
   *
   * @param \Drupal\commerce_order\Entity\OrderInterface $order
   *   The order.
   */
  protected function setOrderLanguage(OrderInterface $order): void {
    $order->setData('checkout_language', $this->languageManager->getCurrentLanguage()->getId());
  }

}

I will also open an issue on the Drupal Symfony Mailer module (which includes the CommerceOrderEmailBuilder), which will include the following patch:

From 6cd4e1fbf60ad99d6673996b50fd0a5566fa5cb0 Mon Sep 17 00:00:00 2001
From: Trent Crawford <trent.crawford@liip.ch>
Date: Mon, 6 Jan 2025 14:47:35 +0100
Subject: [PATCH] Set the language based on the checkout language.

---
 .../CommerceOrderEmailBuilder.php             | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/src/Plugin/EmailBuilder/CommerceOrderEmailBuilder.php b/src/Plugin/EmailBuilder/CommerceOrderEmailBuilder.php
index 601cc8b..656833b 100644
--- a/src/Plugin/EmailBuilder/CommerceOrderEmailBuilder.php
+++ b/src/Plugin/EmailBuilder/CommerceOrderEmailBuilder.php
@@ -5,6 +5,7 @@ namespace Drupal\symfony_mailer\Plugin\EmailBuilder;
 use Drupal\commerce_order\Entity\OrderInterface;
 use Drupal\commerce_order\Entity\OrderType;
 use Drupal\symfony_mailer\Address;
+use Drupal\symfony_mailer\AddressInterface;
 use Drupal\symfony_mailer\EmailFactoryInterface;
 use Drupal\symfony_mailer\EmailInterface;
 use Drupal\symfony_mailer\Entity\MailerPolicy;
@@ -92,6 +93,27 @@ class CommerceOrderEmailBuilder extends EmailBuilderBase {
       ->setVariable('order_number', $order->getOrderNumber())
       ->setVariable('store', $store->getName());
 
+
+    // This is a workaround to set the email language based on the
+    // order checkout language.
+    if ($checkout_language = $order->getData('checkout_language')) {
+      // Calling setTo() above is still required to ensure we get the
+      // list of addresses.
+      $to_addresses = $email->getTo();
+      // In this case we likely only have one to address.
+      foreach ($to_addresses as $index => $to_address) {
+        if ($to_address instanceof AddressInterface) {
+          $to_addresses[$index] = new Address(
+            $to_address->getEmail(),
+            $to_address->getDisplayName(),
+            $checkout_language,
+            $to_address->getAccount()
+          );
+        }
+      }
+      $email->setTo($to_addresses);
+    }
+
     // Get the actual email value without picking up the default from the site
     // mail. Instead we prefer to default from Mailer policy.
     if ($store_email = $store->get('mail')->value) {
-- 
2.47.1

I will open an issue on commerce core with an MR:

From 344ef06b869b9535b2b558df380deb1146a0a5c7 Mon Sep 17 00:00:00 2001
From: Trent Crawford <trent.crawford@liip.ch>
Date: Mon, 6 Jan 2025 16:29:52 +0100
Subject: [PATCH] Send the email in the checkout language if available.

---
 modules/order/src/Mail/OrderReceiptMail.php | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/modules/order/src/Mail/OrderReceiptMail.php b/modules/order/src/Mail/OrderReceiptMail.php
index 67affa1e..ff0db9c7 100644
--- a/modules/order/src/Mail/OrderReceiptMail.php
+++ b/modules/order/src/Mail/OrderReceiptMail.php
@@ -43,6 +43,8 @@ class OrderReceiptMail implements OrderReceiptMailInterface {
     $order_type = $order_type_storage->load($order->bundle());
     $subject = $order_type->getReceiptSubject();
     if (!empty($subject)) {
+      // @todo Support token replacement in the checkout language.
+      // @see Drupal\commerce\MailHandler::changeActiveLanguage().
       $subject = $this->token->replace($subject, [
         'commerce_order' => $order,
       ]);
@@ -73,6 +75,11 @@ class OrderReceiptMail implements OrderReceiptMailInterface {
       $params['langcode'] = $customer->getPreferredLangcode();
     }
 
+    // Use the checkout language if available.
+    if ($checkout_language = $order->getData('checkout_language')) {
+      $params['langcode'] = $checkout_language;
+    }
+
     return $this->mailHandler->sendMail($to, $subject, $body, $params);
   }
 
-- 
2.47.1


I am welcome to any feedback on the approach.

The commerce email module will potentially also need an update for this.

🇨🇭Switzerland tcrawford

I am not sure the checkout completion event is a reliable point in time to set the language, as order completion can occur in the IPN/webhoook and the user may not return to the site.

Order completion is also not reliable as the completion may occur in the webhook, where the language context is not available (as not all payment gateways provide a way to set the callback URL per order).

🇨🇭Switzerland tcrawford

I am also seeing the issue periodically. Looking in the logs, I am currently not seeing "PluginNotFoundException" (although we have in the past in connection with this issue). What I am seeing is the out of memory issue. Therefore, I am linking this to the existing issue #3303674. I am not 100% positive they are related, but it provides context.

🇨🇭Switzerland tcrawford

I seem to be having the same symptom. I am using symfony mailer lite. I suspect this is related. I will update if I find the cause.

🇨🇭Switzerland tcrawford

The issue was caused by another module 'responsive_preview'.
Disabling this module resulted in the module working as expected.
Closing as 'cannot reproduce'.

🇨🇭Switzerland tcrawford

This same issue occurs at other places. I will add some commits to the above MR.

🇨🇭Switzerland tcrawford

I am just following up again on this issue, are you willing to work with me so I can help improve the module?

🇨🇭Switzerland tcrawford

I am looking at a cleaner way to implement this, so re-assigning the issue to me.

🇨🇭Switzerland tcrawford

I have added test coverage. Thanks in advance for a code review and community testing.

🇨🇭Switzerland tcrawford

You can also user massageFormvalues() to set empty strings to null, which is the approach I took in a custom widget.

🇨🇭Switzerland tcrawford

@mondrake You are right. The issue only occurs for me in projects using the drupal/rokka module, which uses its own RokkaToolkit that extends the GDToolkit. Rokka supports the svg extension, but there is no related type and so we see the type error.

  /**
   * Get the mime type.
   *
   * @return string
   *   Mime type.
   */
  public function getMimeType(): string {
    $mime_type = $this->getType() ? image_type_to_mime_type($this->getType()) : '';

    // GD library does not support SVG, but Rokka does. So we will trick the
    // toolkit and detect SVG as a mime time the first time the file is uploaded
    // to the tmp folder. If we detect an SVG image, we allow the upload.
    if (empty($mime_type)) {
      $uri = $this->getSource();
      if ($uri && file_exists($uri) && strpos($uri, 'rokka://') === FALSE) {
        $rokka_additional_allowed_mime_types = ['image/svg', 'image/svg+xml'];
        $mime_type_guessed = mime_content_type($uri);
        if (in_array($mime_type_guessed, $rokka_additional_allowed_mime_types)) {
          $mime_type = $mime_type_guessed;
        }
      }
    }

    return $mime_type;
  }

I guess we need to get have the Rokka Toolkit modified to override the load() method also and check that getType() is set.

I am closing this issue as 'cannot reproduce' as it only occurs in combination with the RokkaToolkit.

🇨🇭Switzerland tcrawford

Thanks John. I am not sure we need the test to use the CRON. The CRON just runs a services to queue expired carts for deletion and the queue worker just uses the storage to delete them, so we could test by just deleting the carts programmatically. We can update the "steps to reproduce" to include "programmatically delete a cart" if you would like. I can write a test for that as time permits.

Production build 0.71.5 2024