Contrib.social

Make it possible to opt out of creating a form_id per product_variation in add to cart

Open on Drupal.org →
Created on 15 April 2024, 2 months ago
Updated 10 May 2024, about 2 months ago

Describe your bug or feature request.

I just created ✨ Make it possible to opt out of form_alter with form_id when running alter for a form Needs work for this exact same reason, but for core. Anyway, core is moving more slowly, and it's very possible to implement this inside commerce without that issue resolved.

The issue is as follows (I am copy pasting from the other issue so I don't have to write everything again):

---

When a form has a lot of variations, storing hook implementations for it might really fill up the cache.

For example, on this site this was a problem for. We use Drupal Commerce. Drupal commerce has a separate form id per variation of the add to cart form. Which is kind of useful in a way, but it ends up creating its own alter hooks for commerce_order_item_add_to_cart_form_commerce_product_1 and commerce_order_item_add_to_cart_form_commerce_product_2 and so on. When this site has more than 20M products, that's a lot of potential alter hooks. And Drupal will cache all of these alter hooks in the cache, along with the implementors of it. Even if it's empty. So if we have 20M forms with different variations of commerce_order_item_add_to_cart_form_commerce_product_xx it all adds up even if its a compressed, serialized empty array.

If a bug, provide steps to reproduce it from a clean install.

To reproduce, create a couple of products.

Visit the first one. and the second one I guess. Now let's inspect the cache bootstrap cache bucket with the module_implements key:

$items = \Drupal::service('cache.bootstrap')->get('module_implements')->data;
$filtered = array_filter($items, fn($k) => str_starts_with($k, 'commerce_order_item_add_to_cart_form'), ARRAY_FILTER_USE_KEY);
var_dump($filtered);

You will see the individual items in there, showing the problem and that it will continue filling up this cache key:

array(5) {
  'form_commerce_order_item_add_to_cart_form_commerce_product_1_alter' =>
  array(0) {
  }
  'form_commerce_order_item_add_to_cart_form_commerce_product_2_alter' =>
  array(0) {
  }
}
✨ Feature request
Status

Needs review

Version

2.0

Component

Cart

Created by

🇳🇴Norway eiriksm Norway

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @eiriksm
  • Comment 2 months ago →
    🇮🇱Israel jsacksick

    I think it is form_commerce_order_item_add_to_cart_form_commerce_product_VARIATION_2_alter no? ("commerce_product_variation" and not "commerce_product").

    Judging by this code:

    if ($purchased_entity = $order_item->getPurchasedEntity()) {
        $this->formId .= '_' . $purchased_entity->getEntityTypeId() . '_' . $purchased_entity->id();
      }

    In any case, I think the way to opt out for this should probably be via a setting (e.g: Settings::get('commerce_cart_extended_form_id', TRUE)).
    This isn't really a setting that belong to a UI IMO as this setting would be primarily used by developers... Ofc the alternative approach is to just swap the add to cart form class and override the gerFormId() class.

  • Comment 2 months ago →
    🇮🇱Israel jsacksick
  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway

    Those IDs are probably correct, yes. The actual changes I had locally were changed for the core issue, so I winged it ;p

    Yes, such a setting was exactly what I had in mind actually. I will post a MR once the queuing of Drupals gitlab catches up, there are some slowness happening today apparently.

  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway

    Actually it's this piece of code, so not actually variations:

    // The default form ID is based on the variation ID, but in this case the
    // product ID is more reliable (the default variation might change between
    // requests due to an availability change, for example).
    $form_object->setFormId($form_object->getBaseFormId() . '_commerce_product_' . $product_id);

    In product ProductLazyBuilders

  • Merge request !247Add a setting for it → (Open) created by eiriksm
  • Status changed to Needs review 2 months ago
  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway
  • Comment 2 months ago →
    🇳🇴Norway eiriksm Norway

    drupal gitlab is slow today, but I did create a MR and I expect it to show here at some point, so the status is now needs review

  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update 2 months ago
    793 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update 2 months ago
    793 pass
  • Pipeline finished with Success
    2 months ago
    Total: 714s
    #147032
  • Comment about 2 months ago →
    🇮🇱Israel jsacksick

    Wondering if we shouldn't use a name for the setting that is a little bit more specific? What if other forms are defined in the future? Just not sure how to make it too wordy....

    commerce_cart_add_to_cart_extended_form_id is a bit too longh...
    Maybe commerce_add_to_cart_extended_form_id? Though here we'd drop the commerce_cart prefix.

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024