The toUrl() method on ProductVariation entities to generate URLs that preselect variations on the product page adds a ?v=PRODUCT_VARIANT_ID GET parameter to the URL. The product variant’s fields are correctly displayed, but the add to cart form doesn’t have the correct attributes selected for that variant.

This bug was introduced in #2860840: Product variation not translated on "add to cart" form . It compares the untranslated selected variation with the list of available varaiations which are translated:

$selected_variation = $this->variationStorage->loadFromContext($product);
// The returned variation must also be enabled.
if (!in_array($selected_variation, $variations)) {
  $selected_variation = reset($variations);
}
CommentFileSizeAuthor
#12 2941834-12.patch9.41 KBmglaman
#9 2941834-9.patch6.09 KBmglaman
#2 2941834-fix-default-product-variation-2.patch1013 bytesluksak

Comments

Lukas von Blarer created an issue. See original summary.

luksak’s picture

luksak
he/him
commented
Title: Add to cart form doesn't use the right default translation when proving the v=ID GET parameter » Add to cart form doesn't use the right default translation when providing the v=ID GET parameter
Status: Active » Needs review
StatusFileSize
new 2941834-fix-default-product-variation-2.patch1013 bytes

This patch translates the selected variation before the comparison.

luksak’s picture

luksak
he/him
commented
Issue tags: +Needs tests

Actually we should write a test that checks if the right attributes are selected when providing the GET parameter.

Status: Needs review » Needs work

The last submitted patch, 2: 2941834-fix-default-product-variation-2.patch, failed testing. View results

bojanz’s picture

bojanz commented

The current fix seems to break current tests.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Issue summary: View changes
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

So we need a test which has

  • A translated product and variations
  • Visit the translated product with the ?v= context
  • Assert fields rendered correctly
  • Assert add to cart selected attributes are selected correctly

So the case here is not that we lost translation... it's just falling back to the default variation.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented 
+++ b/modules/product/src/Plugin/Field/FieldWidget/ProductVariationAttributesWidget.php
@@ -90,7 +90,7 @@ class ProductVariationAttributesWidget extends ProductVariationWidgetBase implem
-    $variations = $this->loadEnabledVariations($product);
+    $variations = $this->variationStorage->loadEnabled($product);

The break is somewhere else.

\Drupal\commerce_product\Plugin\Field\FieldWidget\ProductVariationWidgetBase::loadEnabledVariations actually runs $this->variationStorage->loadEnabled() and then retrieves the translation.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2941834-9.patch6.09 KB

This is a blind test enhancement. PhantomJS is screwing up locally. But this adds more context testing for `?v=`. I just noticed we have no Functional testing for it, only Kernel.

  • Adds coverage in ProductVariationFieldInjectionTest by visiting context (non-i18n)
  • Adds coverage in AddToCartMultiAttributeTest for context
  • Mimic part of main tests in AddToCartMultilingualTest with visiting context

Status: Needs review » Needs work

The last submitted patch, 9: 2941834-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

WE HAVE CONFIRMED FAILURE OF ISSUE REPORTED IN TEST! WOO!

+++ b/modules/cart/tests/src/FunctionalJavascript/AddToCartMultilingualTest.php
@@ -246,4 +268,31 @@ class AddToCartMultilingualTest extends CartBrowserTestBase {
+    $this->assertAttributeSelected('purchased_entity[0][variation]', 'My Super Product - FR Blue, FR Medium');

Typo - should be "Mon super produit"

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 2941834-12.patch9.41 KB

I think the problem is

      // The returned variation must also be enabled.
      if (!in_array($selected_variation, $variations)) {
        $selected_variation = reset($variations);
      }

And that is because \Drupal\commerce_product\ProductVariationStorage::loadFromContext does not return a translated variation, which we decided the storage should not do (that's really a repository's job.

So I moved this to a shared helper which gets the translation from context via the entity repository.

Blind patch and test run.

bojanz’s picture

bojanz commented

Fix looks good!

+    // Load variation2 directly with context and assert injection.

"with context" sounds very confusing here, I didn't connect it to the url at all.
"Load variation2 directly via the url." makes more sense.

Same with the *fromContext test methods, I'd call that *fromUrl

+    $this->config('system.site')->set('default_langcode', 'fr')->save();
+    drupal_flush_all_caches();

We should be able to use $this->rebuildContainer() instead of drupal_flush_all_caches().

  • bojanz committed 4285fc9 on 8.x-2.x authored by mglaman 
    Issue #2941834 by mglaman, Lukas von Blarer: Add to cart form doesn't...
bojanz’s picture

bojanz commented
Status: Needs review » Fixed

Addressed my feedback, committed. Thanks!

Status: Fixed » Closed (fixed)

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