The add to cart form builds form ids by using a static counter to make it possible to have multiple add to cart forms on the same page. It does this in this function:

   // @see Drupal\commerce_cart\Form\AddToCartForm
  public function getFormId() {
    $form_id = $this->entity->getEntityTypeId();
    if ($this->entity->getEntityType()->hasKey('bundle')) {
      $form_id .= '_' . $this->entity->bundle();
    }
    if ($this->operation != 'default') {
      $form_id = $form_id . '_' . $this->operation;
    }
    $form_id .= '_' . self::$formInstanceId;

    return $form_id . '_form';
  }

The AddToCartFormatter provided by commerce_product uses lazy builders to load add to cart forms.

  // @see Drupal\commerce_product\Plugin\Field\FieldFormatter\AddToCartFormatter
  public function viewElements(FieldItemListInterface $items, $langcode) {
    $elements = [];
    $elements[0]['add_to_cart_form'] = [
      '#lazy_builder' => [
        'commerce_product.lazy_builders:addToCartForm', [
          $items->getEntity()->id(),
          $this->viewMode,
          $this->getSetting('combine'),
        ],
      ],
      '#create_placeholder' => TRUE,
    ];
    return $elements;
  }

Drupal's merging logic to keep track of placeholders actually stores the placeholders in reverse order. See Drupal\Core\Render\BubbleableMetadata::mergeAttachments. So when two forms are added to the same page (Form A and Form B), the second one is actually rendered before the first. Which means the static counter for Form A is 2 and the static counter for Form B is 1.

When the form is submitted and Drupal rebuilds the forms, they are (or at least they can be) executed in the original order. When Drupal checks the submitted form ID (Form B-1) against the rebuilt form ID (Form B-2) they do not match. This causes the form not to be submitted. (And on AJAX callbacks it causes it to break).

  // @see Drupal\Core\Form\FormBuilder
  public function doBuildForm($form_id, &$element, FormStateInterface &$form_state) {
     . . .

      // Set a flag if we have a correct form submission. This is always TRUE
      // for programmed forms coming from self::submitForm(), or if the form_id
      // coming from the POST data is set and matches the current form_id.
      $input = $form_state->getUserInput();
      if ($form_state->isProgrammed() || (!empty($input) && (isset($input['form_id']) && ($input['form_id'] == $form_id)))) {
        $form_state->setProcessInput();
        if (isset($element['#token'])) {
          $input = $form_state->getUserInput();
          if (empty($input['form_token']) || !$this->csrfToken->validate($input['form_token'], $element['#token'])) {
            // Set an early form error to block certain input processing since
            // that opens the door for CSRF vulnerabilities.
            $this->setInvalidTokenError($form_state);

            // This value is checked in self::handleInputElement().
            $form_state->setInvalidToken(TRUE);

            // Make sure file uploads do not get processed.
            $this->requestStack->getCurrentRequest()->files = new FileBag();
          }
        }
      }
      else {
        $form_state->setProcessInput(FALSE);
      }

      // All form elements should have an #array_parents property.
      $element['#array_parents'] = [];
    }
   . . .
  }

This solution seemed to work for me but I don't know enough about the Drupal Commerce universe to know if there are other implications of this fix. It does work for me with multiple products and multiple variations on the same page. Also using AJAX. It does assume there's only one add to cart form per variation on the page.

   // @see Drupal\commerce_cart\Form\AddToCartForm
  public function getFormId() {
    $form_id = $this->entity->getEntityTypeId();
    if ($this->entity->getEntityType()->hasKey('bundle')) {
      $form_id .= '_' . $this->entity->bundle();
    }
    if ($this->operation != 'default') {
      $form_id = $form_id . '_' . $this->operation;
    }
    $form_id .= '_' . $this->entity->getPurchasedEntity()->id();

    return $form_id . '_form';
  }
CommentFileSizeAuthor
#13 2886614-13.patch7.34 KBbojanz
#6 addtocartform_form_id-2886614-5.patch1.53 KBmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

onedotover created an issue. See original summary.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

I agree that the counter needs to go.
The reason why I didn't use $this->entity->getPurchasedEntity()->id() originally is because changing attributres changes the variation (purchased entity), so that ID actually changes, which I assumed would cause problems.

onedotover’s picture

onedotover CreditAttribution: onedotover commented

Yeah I was a little concerned about that too. Since it happens early enough in the process I think $this->entty->getPurchasedEntity() is actually just the default variation from the function below. So even though the variation changes, the initial/default value remains the same. It feels pretty sketchy to me, but then again if we're creating the OrderItem based on the default_variation maybe it's not a bad assumption.

  //  @see \Drupal\commerce_product\ProductLazyBuilders
  public function addToCartForm($product_id, $view_mode, $combine) {
    /** @var \Drupal\commerce_order\OrderItemStorageInterface $order_item_storage */
    $order_item_storage = $this->entityTypeManager->getStorage('commerce_order_item');
    /** @var \Drupal\commerce_product\Entity\ProductInterface $product */
    $product = $this->entityTypeManager->getStorage('commerce_product')->load($product_id);
    $default_variation = $product->getDefaultVariation();
    if (!$default_variation) {
      return [];
    }

    $order_item = $order_item_storage->createFromPurchasableEntity($default_variation);
    $form_state_additions = [
      'product' => $product,
      'view_mode' => $view_mode,
      'settings' => [
        'combine' => $combine,
      ],
    ];
    return $this->entityFormBuilder->getForm($order_item, 'add_to_cart', $form_state_additions);
  }

I was also considering making it dependent on the product id. Both scenarios will introduce issues if the same product appears on the page twice though.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Assigned: Unassigned » mglaman

Reviewing today. We have multiple sites using Big Pipe and no cart issues.

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

I can confirm that this problem exists... it was a problem originally, was then solved by using the product ID. But that being a problem for generic purchasable entities, introducing the counter in http://cgit.drupalcode.org/commerce/commit/modules/cart/src/Form/AddToCa... re-introduced the problem for us as well.

The dangerous thing here is that it really seems to only be triggered on lazy loading. We didn't saw it at first, when the mentioned commit caused this problem at a client site because on the main view - whose cache metadata seems to be cheap - we hadn't got the problem, but on other product related views there was exactly this problem, where the cache metadata is obviously more expensive, so that the lazy loading took place. I saw that quite a long time after that Commerce update, only by accident. So obviously no one has ever bought wrong products because of this, but the problem existed

PS: so if you define a view with expensive cache metadata, like cache per user id or something like that, you should easily trigger this problem

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Needs work » Needs review
FileSize
addtocartform_form_id-2886614-5.patch1.53 KB

Here's a proposed fix. Using \Drupal\Component\Utility\Html::getUniqueId to help prevent conflict instead of internal counter.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

Also,

    $form_id .= '_' . $this->entity->getPurchasedEntity()->id();

This assumes the order item has a purchasable entity set / allowed.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

Okay, I have an experiment up on https://github.com/mglaman/commerce/tree/cart-big-pipe. I could not reproduce the error manually at all. Nor via tests. I have added a branch on my sandbox to test Big Pipe + the add to cart formatter. More specific ways to reproduce would help.

The only thing I can imagine is that on XYZ conditions not all of the forms are rebuilt. I have not tested attribute AJAX with this, only direct add to cart submission.

EDIT: Only thing I can think of is when the form ID rebuilds on AJAX, it resets? so it returns a proper form ID but internall it's screwed up.

onedotover’s picture

onedotover CreditAttribution: onedotover commented

Yes, that's exactly the issue. Both the static counter and the HTML::getUniqueId() approaches make the form ID dependent on the order the forms are loaded. When using AJAX submit the forms on the page are rebuilt. Lazy Loading causes the rebuild order to be different than the original page load which causes the form IDs not match up.

agoradesign’s picture

agoradesign CreditAttribution: agoradesign commented

The most solid way would be, if we can get the product entity id (not the variation one!) into the form ID, by still supporting any purchasable entities.

ad hoc I can think of 3 ways to do this, each of them having a small "elegance flaw":

  • introduce an additional function in the PurchasableEntity interface such as getParentId() (or any better) name, and used this, if a value is provided. Product variations should return the parent product ID here.
  • allow external manipulation by firing an event or implementing the resolver pattern. either way, commerce_product should react, if the purchasable entity is a product variation
  • the form class checks directly, if the purchased entity is implementing ProductVariationInterface, and only adds the product entity ID in this case. For sure the less bloated, but ugliest of all three ideas

The only problem at the end would be, if we have the same product displayed twice (or more often) on the same page. But this edge case would have to be solved now anyway and having the correct product identified is the first step

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

oneddotover, so theoretically this PR and new tests will always pass without JS: https://github.com/drupalcommerce/commerce/pull/756. But once I add the same test in FunctionalJavaScript and manipulate attributes, it should break.

drugan’s picture

drugan CreditAttribution: drugan as a volunteer commented

@mglaman

You're right, modules/cart/tests/src/Functional/MultipleCartFormsTest.php is useless without javascript.

An attempt was made to move this test to modules/cart/tests/src/FunctionalJavascript namespace.

See an example on the #582 PR and the file.

This test was keeping green a couple of months but for now it fails. The DC evolves so rapidly that I am not able to support all my PRs up to date. Tests are sacrificed to the actual functionality requested on a PR. Now I am waiting for the first RC to make some spring cleaning on my PRs.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Title: AddToCartForm Form id issues with LazyLoading » Rework the AddToCartForm form ID handling
Priority: Normal » Critical
FileSize
2886614-13.patch7.34 KB

Retitling.

onedotover's comment helped me finally understand the problem. I built the static counter solution assuming that the form ID won't be regenerated on #ajax, but it actually does get regenerated. We can't rely on a counter because we can't rely on the forms always being built in the same order. We need to use a stable ID.

I now understand why using the variation ID worked, the form is always initially built with the default variation, so the ajax change doesn't matter.

Still, the main problem is the fact that many months after this bug was discovered, we still don't have a failing test. Matt is working on a failing test, and I'm uploading an initial patch in the meantime.
It does the following:
1) adds the purchasable entity type ID to the ID, to prevent collisions between add to cart forms for different purchasable entity types
2) uses the purchasable entity ID by default (variation ID, bundle ID, etc)
3) for products it overrides the form ID with one that uses the product ID (cause it's more stable)
4) allows for other overrides using the setter (for example, when outputting an add to cart form with no purchasable entity for each node, one can use the node ID)

drugan’s picture

drugan CreditAttribution: drugan as a volunteer commented

The #13 did not work for me.

I still observe this fatal error:

InvalidArgumentException: Unknown attribute field name "attribute_mysizesattribute". in Drupal\commerce_product\Entity\ProductVariation->getAttributeValueId() (line 272 of modules/contrib/commerce/modules/product/src/Entity/ProductVariation.php).

To reproduce follow the guide on this comment except applying this patch:

wget https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pull/582.diff && git apply 582.diff

Instead apply the patch on the #13 comment and try add to cart any variation of the My Colors & Sizes - FIRST product.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

@drugan
I don't think your error is related to this problem, I think it's a completely different issue.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

drugan, your error isn't due to the form ID, quite exactly. I just tested it out. During the AJAX call and submission something odd happens where the following errors

    if (!in_array($field_name, $attribute_field_names)) {
      throw new \InvalidArgumentException(sprintf('Unknown attribute field name "%s".', $field_name));
    }

The requested field name isn't part of the current bundle. So maybe it does tie in, and having three variation types: one with color, one with size, one with both, mixed on a view provides easy way to solve.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

onedotover, are you able to provide the View you're using?

onedotover’s picture

onedotover CreditAttribution: onedotover commented

The setup I'm using is just a node with a product field (multiple values allowed). I have implemented a form_alter to make the Add to Cart button use AJAX. The issue pops up when there is more than one product on the page.

I'm going to try to test out #13 this morning. Would that be helpful? Let me know what else I can do.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

Yeah can you test #13? I think we need to just commit that (verified a fix on a production site.)

And I'd like to also commit https://github.com/drupalcommerce/commerce/pull/756 which greatly expands our test coverage (but still fails to report the main bug.) However we need to pick battles, and a confirmed fix with more test coverage, but still possible edge case is something we'd have to live with for the moment.

onedotover’s picture

onedotover CreditAttribution: onedotover commented

#13 works great. Tested with multiple product AJAX forms on the same page. It also has the added benefit of identifying add to cart forms with a prefix "commerce_order_item_add_to_cart_form*" rather than having to regex.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Assigned: mglaman » Unassigned
Status: Needs review » Reviewed & tested by the community

Okay, we'll commit #13 as verifed it fixes a bug reported on two production sites.

I'll open a follow up to address drugan's issue of multiple add to carts with different variation types and attributes.

I will commit https://github.com/drupalcommerce/commerce/pull/756 as tests for this issue (didn't cause a fail, but added _a lot_ of big pipe / cart coverage.)

  • mglaman committed 19445cc on 8.x-2.x authored by bojanz 
    Issue #2886614 by mglaman, bojanz, onedotover, agoradesign: Rework the...
  • mglaman committed f37464b on 8.x-2.x
    Issue #2886614 by mglaman: Expand test coverage for add to cart forms.
mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Reviewed & tested by the community » Fixed

Tests and fix committed! Thanks everyone.

Opened #2893182: Multiple add to cart forms with different variation types throws exception and started writing test.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

Fix attribution.

drugan’s picture

drugan CreditAttribution: drugan as a volunteer commented

@onedotover

Could you please to upload a database dump of your site with example nodes (with products) created. Also, it will be useful if you upload the patch reflecting your "form_alter to make the Add to Cart button use AJAX".

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.