Incorrect display of attribute field values on the Add To Cart form

Any errors in your browser console?
How about Drupal's watchdog (admin/reports/dblog)?

I think I found the problem.

In the formElement we set a 'selected variation' value (this is by default the first value of the variations).
When we change e.g. the size, the value is not updated with the new variation id and so we will return our default variation in our Ajax callback.

I'm trying to create a patch for this one.

I've noticed that ProductVariationAttributesWidget::formElement() is called twice when the first change is made, but only once for every subsequent change.

When the first change is made the first time formElement() is called it returns the correct variation, however the second time it is called it returns the default variation, and the user only ends up seeing the default variation.

So the first change displays incorrectly, however subsequent changes display correctly.

Also if a variation doesn't exist which matches the options the default is returned, it would be preferable if the first match that contains the option which was changed was returned instead.

This is definitely a problem for me. I don't get any javascript or watchdog errors.

Here is another example that might make it more clear.

* Variation has two attributes, Gem color and gem type.
V1 is Red, Emerald
V2 is Red, Ruby
V3 is Blue, Topaz
V4 is White, Topaz

now if the field order is Gem Type, Gem Color you see this for options

Emerald
Red << selection disabled, since all emeralds are Red.

and you can switch from Emerald to Ruby, and you see this:

Ruby
Red << selection disabled, since all rubies are Red.

switch to topaz:

Topaz
Blue << can now select to White

In this configuration we can access all our variations via the form.

Now we change the attribute order to Gem Color, Gem Type and our form shows:

Red << options are Red, Blue and White
Emerald << select options are ONLY Emerald and Ruby, (since these are types of the only Red variations.

Should we be able to switch to topaz for the Gem type? There are Topaz variations of this product, just not in red. This might be OK if you see Topaz as an option when you switch from Red to Blue or White, but that doesn't happen! In stead, the form is broken and you can't get to the white or red topaz variation.

@ransomweaver

I've tried your "Gems" use case in both normal and reversed attributes order and seems the solution already is available for you.

First, apply patch linked on this comment.
Then, apply patch linked on this comment.

As a bonus to correctly displayed attributes you'll get combined attributes possibility and properly working multiple products (add to cart forms) on a page, for example with views pages or blocks.

Thank you drugan, the patches are an improvement. The patches do make the variation selection work.

However, it does not make the *selectable* attribute menus change to indicate the correct value for the new variation.

Maybe this is an unrelated problem, to do with the ajax load of the variation, but I will link here an example:

http://prelive.shopinde.com/product/1965?v=5114

There are 4 variations of this pearl ring;
white gold, size 6, Freshwater, white
white gold, custom size, Freshwater, white
yellow gold, size 6, Tahitian, Black
yellow gold, custom, Tahitian, Black

Crucially, there are is NO tahitian, white variation, and the yellow gold is ONLY Tahitian black. The problem here is that when you change from White gold to Yellow gold, the Pearl Type menu does not change from Freshwater to Tahitian.

Interestingly, it DOES change the pearl color from white to black, which was not a selectable option. And why wasn't it a selectable option? because there are no black pearl + white gold combinations. But there also no Tahitian + white gold options, and that IS selectable. What is the difference? I am intrigued.

I know it looks a bit weird to have size and color attribute select lists above metal and pearl type attributes but keeping in mind the complexity of your product it might be a possible solution for the issue. The rule of thumb is following: first you need to place attributes which is common for all variations and then more specific attributes (given the #8 patches are applied).

The Pearl Ring variations created:

Note that gold type and pearl type attributes are always disabled and change their value only when the color attribute is changed. If you change size attribute then nothing happens because it is common attribute for all the variations.

@ransomweaver By the way, great site in your example. Is it possible to say what the theme you are using?

Thanks Drugan, thats a good explanation of what's going on, and for investigating so thoroughly. I think the problem is that we won't know what permutations of attributes the seller is going to use for the product/variation type, which in this case is "pearl_ring".

As you see, attributes on pearl_ring are
ring_size
metal
pearl_type
pearl_color

What if we add to this variation assortment

custom, golden, 14k white gold, southsea

I think it still works....

We have about 20 different product types, like "diamond_pendant" with different attributes like chain_length, chain_type, diamond_carat_weight, diamond_color etc.

Probably each variation type has a certain ordering of attributes that works best. Maybe its not even possible for pearl_ring to be broken in this ordering. I will experiment.

The theme is a subtheme of bootstrap contrib theme. Thanks for the compliment!

No, custom, golden, 14k white gold, southsea does not work even after reordering attributes like the following:

1. Color
2. Pearl Type
3. Gold Type
4. Size

There are almost countless variation attributes use cases and you need to test each case individually reordering attributes after adding new variations (given #8 patches applied). I'd recommend to read #2730643: Product variation attributes shows combines variations that not exist.. It has a lot of contributed images demonstrating the issue.

The actual magic does happen in modules/product/src/Plugin/Field/FieldWidget/ProductVariationAttributesWidget.php->selectVariationFromUserInput(). When you click an option it takes all the variations saved on a product as the first argument and $form_state->getUserInput() as the second and checks in the loop if the IDs of attributes in user input are equal to ones in a product variation. If it is - then this variation is picked up as selected variation. Pretty simple and works great if you created all the possible (or most of them) variations on a product. But things become messed up as in your case or even worse if you have optional attributes on a product variation type. The reason is that drupal ajax ignores fields which have only one option (disabled) or having _none option chosen thus returning only meaningful attribute options IDs. In your latest example only two of them are returned: color and size attributes IDs. How to detect what the variation was selected in this case? Without #8 patches applied it returns the first variation in the list if it didn't found the exact attributes combination. My patch attempts to detect variation which has at least some IDs in the user input and returns it.

The whole issue is not the commerce issue and can only be fixed on the core which is pretty difficult because the use case as in your example will be marked as specific and the fix will be works as designed.

Thanks @drugan,

I can see now how the golden/southsea breaks the form, and actually prevents the access to the Black/tahitian.

I wonder if, in ProductVariationAttributesWidget::formElement, a path-finding algorithm or similar could be used to verify that in the current order of processing the attribute fields, all variations are reachable, and if not, change the field order until they are. I don't think this would result in the order changing every time the user selects an option, but would mean a difference between the order of fields for product A vs product B if for A the default order works and for B it does not.

Possible solution for complex attributes' use cases like on the issue summary or #9 and #11 comments.

UPD: Look for the solution on the #51 comment.

For the test I've used #11 example. Now it works for whatever attributes' order:

Note that attribute fields having only one option are disabled because they have no any alternative choice.

Also, to test the solution I've created variations shown on the issue summary. Taking into account the complexity of this example it is recommended to move to the top attribute fields which have most options for the given set of variations. For example if you have Red, Green, Blue 3 colours attribute options and Large, Medium 2 size options used (sic!) in product variations then better to have colours at the top and sizes at the bottom. Go to admin/commerce/config/product-variation-types/MY-VARIATON/edit/form-display and reorder the attribute fields.

Now it works for the following attributes' order:

1. Colours (6 options)
2. Sizes (1 option for each colour)
3. Pack size (1 option for each colour)

UPD: Now it should work for any attributes order.

If you have use cases which don't work with the current solution please post them here so we together will try to find better fix. And don't forget to report what modes of attributes order you've used in your test.

Added more images for #14

Better and Better, drugan!

I am having good results with your latest. I do see that if I leave my attribute order as follows:

ring size
pearl color
pearl type
metal

It does make available all combinations, but If I do the following
1) change to custom ring size, which loads, white + Freshwater, but also makes available Golden + South sea
2a) change to golden, it does not change pearl type to south sea, and leaves an unavailable Freshwater in the pearl type
or 2b) change to south sea, it does not change color to Golden, and leaves unavailable White in the pearl color.

a similar problem occurs with
metal
pearl type
color
ring size

I will keep testing, but this seems a good step forward.

@ransomweaver

Now it works for your #11 case in what ever attributes order. If you applied over_545_575_582.patch I'd recommend to revert it and then to apply over_545_575_582-3.patch.

For the rest of the folk who is looking for the fix - please read #14 comment.

Also, changing the issue status to Needs Work in order to get possibly more feedback on the suggested solution.

Hi @drugan,

Its pretty good. The combos that still don't work are

metal
color
size
type

metal
type
size
color

I haven't tried changing the order of the variations in the product. Could that have any effect?

@drugan fyi your patch in #18 no longer applies to latest commerce dev.

Thank you for the feedback as I have no much time to test all the possible attributes use cases.

Please, try the patch on this comment. For me it works with your #19 combos.

@ransomweaver
Not sure about your notice on the #20 because what I've done is this:

Hi @drugan,

Looks good, I can't break it now, in any configuration of attributes. I'll see if I can make any other variation combinations that cause trouble!

the latest patch applies now. Fyi, I am installing patches with composer, like this in my composer.json:

  "patches": {
            "drupal/commerce": {
                "Allow combined attributes": "https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pull/575.patch",
                "AddToCartForm should support all purchasable entities": "https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pull/582.patch",
                "Product variant bulk creation": "https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pull/545.patch",
                "Incorrect display of attribute field values": "https://www.drupal.org/files/issues/over_545_575_582-3.patch"
            }
        }

Set status to active, I'll let @drugan decide what that should be.

I am also patching my drupalcommerce installations with cweagans/composer-patches which is a great addition but in the screenshot above I've demonstrated that patches are applied cleanly.

As for the issue status it cannot be set Needs Review even if you and may be some other users will report of the solution working for them. It can be done just as a follow up after (if) the minimum dependency - #2755529: Product variant bulk creation - will be commited.

Let's wait and see. In the end of February drupalcommerce gets its first release candidate and hopefully more users who are inadvertently encounter the displaying attributes issue and end their search on this page. I think that for now drupalcommerce lead maintainers postponed the issue because they are not sure how much users will experience it. If the number of reporters will be one or two then the whole issue will be marked as specific. Moreover the possible solution is already available for some of cases.

The solution was mostly rewritten. Read #14 comment for more info.

Oops, too many related issue references...

I have had to remove the bulk variation creator patch, and therefore also this patch, due to a memory exhaustion error for some products.

PHP Fatal error: Allowed memory size of 1287651328 bytes exhausted (tried to allocate 20480 bytes) in /modules/contrib/commerce/modules/product/src/ProductVariationBulkCreator.php on line 361

The error occurs with the bulk variations patch applied and with or without this issue's patch.

Interestingly, it seems to happen to products (but not all products) that have only one variation.

I see that this patch depends on bulk variation creator for this method:

+ $creator = \Drupal::service('commerce_product.variation_bulk_creator');
+ if ($all = $creator->getAllAttributesCombinations($variations)['combinations'])

I don't have any need for bulk variation creation, and it seems the most edge case problem of all the patches here. Could we remove this getAllAttributesCombinations from that service and put it somewhere else so this patch can work without it?

Can't reproduce #28 error.

I have 256MB memory_limit on my localhost server and I've tested with one variation created:

1. An example product on #11 comment.
2. A product on this issue summary.
3. A product on the #2730643: Product variation attributes shows combines variations that not exist.

@ransomweaver

What is the memory_limit on your server?
What the product with which attributes you've used?
What was the order of attributes' fields?
Was the single variation active/enabled?

Could we remove this getAllAttributesCombinations from that service and put it somewhere else so this patch can work without it?

No, the collection of all attributes' combinations is the basic thing for this solution and without it I can't imagine how this and #2730643: Product variation attributes shows combines variations that not exist. issues might be fixed. How it works:

First, we need to detect the variation selected by a user when they click at some option on an attribute select list. The problem is that the drupal ajax system ignores the fields having only one option (these fields are disabled and therefore not clickable) and optional fields having - None - option chosen. The question is: how to detect the variation selected if you have, let say, only two attributes' IDs returned out of four available? For now the only answer for this question is to use the variation bulk creator.

Second, based on this selected variation we need to construct new Add to Cart form with unique combination of attributes. That means if you have Red on the first Color attribute then on the second Size attribute you'll get only those values which also have Red in the current combination and so on till the last of attribute fields. Again, keeping in mind the complexity of some attributes' use cases this can be done when we know all possible attributes' combinations returned by the variation bulk creator.

Not sure if it is related with the memory_limit issue but nevertheless I'd recommend to check the following:

1. Update your drupalcommerce installation: composer update drupal/commerce --with-dependencies
2. Reapply all the patches listed on #14 comment.
3. Run update.php then flush caches to ensure that all the system is updated.
4. Go to admin/reports/status and check if you have any mismatched entity and/or field definitions and then update the field(s). Ignore the warning that field can no longer be changed. Just click Save field settings and check once more on the status report page.

@drugan, I think I didn't express myself well; I wast thinking that getAllAttributesCombinations seems like a general purpose method, that needn't be part of batch variation creation, and if it were not part of that, there would be no necessity of having that patch in order to use this patch with getAllAttributesCombinations, which I understand is necessary to how this works.

I updated commerce, made sure of entity updates etc, to no effect. The problem occurs on the local dev setup with 500MB memory_limit and the live server with 2000MB as well.

Here are some examples of products that won't load with the batch variation creation patch applied:

single variation:
http://prelive.shopinde.com/product/1234?v=3720
http://prelive.shopinde.com/product/2047?v=5279
http://prelive.shopinde.com/product/2178?v=5496

multiple variations
http://prelive.shopinde.com/product/2881?v=7760

I suspect that it fact it is getAllAttributesCombinations that is causing the problem, in part because the among the attributes of the products affected, there is a diamond carat weight attribute that has 10000(!) values. 0 to 10 carats in 0.01 increments, which works great as a form display autocomplete field. But that's gonna hurt with any attempt to get every combination. Example, the last two links above.

But I still get the problem with an assortment of attributes (metal, gem color, gem type, chain type, chain length) that maybe shouldn't be a problem. I wonder if it gets caught in some kind of loop.

This works fine in both situations:
http://prelive.shopinde.com/product/1141?v=1505
http://p2p.ransomweaver.com/product/1141?v=1505 << site with patches applied

this does not:
http://prelive.shopinde.com/product/1260?v=3952
http://p2p.ransomweaver.com/product/1260?v=3952 << site with patches applied

I may have to study a spreadsheet closely to see what is different about those two products. They do both have quite a few variations, but they are also quite similar.

If you look at the /shop page http://prelive.shopinde.com/shop/all you can see from the facets what all the possibilities of attributes are. Category, Brand, and Collection are not attributes. The above-mentioned Diamond Carat Weight is not a facet.

And if you want to fill a product attribute with thousands of values, this will do it: ;)

$val = 0.01;
  $weight = 0;
  while ($val <= 10) {
    $strval = '' . $val;
    $attributeValue = \Drupal\commerce_product\Entity\ProductAttributeValue::create(
      [
        'attribute' => 'diamond_carat_weight',
        'name' => $strval,
        'weight' => $weight
      ]
    );
    $attributeValue->save();
    $val += 0.01;
    $weight++;
  }

@ransomweaver

I see some of your example products get into endless loop but I need more info on these products:

1. Please, post screenshots for each product attribute like this.
2. Post the actually created product variations' screenshot like this. Additionally it would help if you post the last variation expanded by clicking Edit button so I know the attribute fields order (+required/optional?) on the variations.
3. Post a patch on your dynamically created in code product attribute with thousands of values so I know exactly where did you put the #30 snippet and at what conditions did you run it.

The solution on #14 is suggested in a hope that it will work against the drupalcommerce HEAD and any further drupalcommerce code customizations have to be adjusted to work with the solution or applied without it otherwise.

@Londova

I understand the need to keep the code as lean as it possible and open for any kind of advice how to fix the attributes issue without bulk creator. By the way, thanks for your example on the issue summary which is a pretty good test case for any of the solution that will be suggested.

Talked with Bojanz about this at Dev Days in Milano last summer.

The current Add To Cart form implementation expects that all possible combinations of attributes are available. This is indeed solved by the variation bulk creator patch #2755529.

But there are situations that a Store has a different situation . With the T-shirts example: a store only has one 'red shirt, size S' and one 'green shirt, size L'. Other attribute variations will never be there.
For this case the current Add To Cart form implementation will never be sufficient.

my 2c
We need an alternative for the current Add To Cart form implementation. At least a simple dropdown-list with variations that are available.
As nice-to-have, it would be great when Add To Cart forms can tell by themselves that they can solve the set of product variations. That way you can provide multiple and it would fall-down in the end to the dropdown-widget if non other Form could solve it.
As minimum the simple-dropdown add-to-cart should be in the list variations formatters on admin/commerce/config/product-types/default/edit/display [Add to cart form, Label, Product attributes overview, ...]

Hi @drugan,

Rather than post screenshots if my attribute values, I am attaching a txt file with the values for each attribute, exported from the db like this:

select name from commerce_product_attribute_value_field_data where attribute='[attribute_name]' order by weight"

The exception is diamond_carat_weight, which is huge. As for that, the script in #30 is not used in my codebase, its a one-off script used to generate the DB values in commerce_product_attribute_value_field_data for the diamond_carat_weight product attribute. I posted it in case you wanted to generate test data in the same way.

I'm also uploading the screenshots you asked for with the product id (as shown in the links in #30 in the filename.

Thanks.

@ransomweaver

Please revert the over_545_575_582-4.patch and apply the patch posted on this comment.

What is done is attempt to reduce memory consumption and optimization by invoking bulk creator just once to generate all the possible combinations of attributes. But anyway, I am not sure if that will work with products having 10 000 options attribute like on #30 comment. Add to this attribute two more attributes with, let say, 5 options on each and in the output you'll get:

10 000 x 5 x 5 = 250 000 combinations each consisting of three field_name => ID pairs

Depending on how much you've created variations on this product, additionally to this huge array you'll get some number of variation instances which are also pretty heavy objects, I'd say.

@ndf

Actually, the issue of mutually exclusive variations is fixed by the #14 solution. See .gif with your T-shirts. Also check this comment and see more .gif demos.

@all

If the #14 solution does not work for your particular case, please see instruction of how to describe your case on the #32 comment. Or, if for some reasons you can't post screenshots here, please describe your case like this:

I have Myproduct with the following attributes and their values:

Color (required):

• Red
• Green
• Blue

Size (optional):

• Large
• Medium
• Small

I've created 3 variations:

• Red, Large
• Green, Large
• Blue

Here describe what is wrong with displaying your attributes on the Add to Cart form ...

@drugan,

Patch 5 makes it so that all the problem products can be viewed! That's great. But... the same products that couldn't be viewed/edited before, still can't be edited.

It looks to me like certain of my variation bundles have this problem and not others, such that none of the products of that variation type work with this patch.

1) none of my variations that have the diamond_carat_weight attribute will /edit. That's probably the 10000 values in this attribute.

2) But the gemstone_pendant variation bundle doesn't work either: http://p2p.ransomweaver.com/product/1234. This is the bundle that has the most attributes, unlike e.g. a gemstone_earrings, which has no problems that I can tell: http://p2p.ransomweaver.com/product/1178

@ransomweaver

What I see on the http://p2p.ransomweaver.com/product/1234 is that it has only one option on each of the attributes. Seems that you have only one variation created on this product. If that is right, then it is expected behaviour just because you have no any alternative choice (one attributes combination == one product variation).

Also, if you have any optional attribute fields on the not working products, please reapply bulk creator patch as today I've made some modifications on the patch which are important for proper handling optional attribute fields.

Hi @drugan, I don't understand your last remark about expected behavior; behavior of what? I agree that there can be only one combination with one variation. But it certainly can't be expected behavior that the edit page won't load if it only has one variation.

In any event, the problem is not restricted to single-variation products. This one and all the other gemstone_pendants are the same in not being able to access the edit page: http://p2p.ransomweaver.com/product/1260

None of the products have optional attribute fields.

@ransomweaver

Please reapply bulk creator patch as it rewritten to reduce memory consumption on a product edit form.

Also, revert the over_545_575_582-5.diff and apply the patch posted on this comment.

The variation bulk creator mostly rewritten in order to comply with performance requirements when using variations with a huge number of attributes combinations. 1 007 160 combinations is tested and actually they are working on the product view page though might be slow on the product add/edit page. For that case autocompletion and autocreation on a product add/edit form might be disabled. Read more here:

#2755529: Product variant bulk creation

Please reapply bulk creator patch and revert the over_545_575_582-6.diff and apply the patch posted on this comment.

Yes @drugan! It works very well, and with your latest on the variant bulk creation patch, it doesn't have any problem with add/edit form.

For the logged in admin, it takes ~2.8 sec to load the view display, vs ~ 0.5 sec for without the patches, but no difference when cached.

Thanks!

I think the patch might need to be rebased, using the latest git version doesn't apply patch.

@dubcanada

Right now I've applied all the four patches cleanly against the latest commit:

Note that it is recommended to apply patches for this solution in the following order:

1. https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul...
2. https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul...
3. https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul...
4. https://www.drupal.org/files/issues/over_545_575_582-7.patch

@drugan modules/cart/tests/src/Functional/MultipleCartFormsTest.php has changed, so https://patch-diff.githubusercontent.com/raw/drupalcommerce/commerce/pul... doesn't apply anymore.

@ransomweaver

Please, try to apply the .diff now...

modules/cart/tests/src/Functional/MultipleCartFormsTest.php file is restored in order to have less conflicts with the PR when the drupalcommerce code changes. So, now we have one more modules/cart/tests/src/FunctionalJavascript/MultipleCartFormsTest.php. Obviously, one of the files should be removed if the PR will be decided to commit. But for now just as a quick solution this PR may exist with two test files targeted at the same functionality.

@drugan success! thanks.

The patch above is modified to support optgroups on the dropdown select list if an attribute is a group of multiple attributes combined into one.
The #14 list of patches is updated.

@see https://developer.mozilla.org/en/docs/Web/HTML/Element/optgroup
@see https://www.drupal.org/node/2842961#comment-12022736
@see https://www.drupal.org/node/2831739#comment-12080972

Hi @drugan,

I need to update Commerce, and PR 582, PR 575 and over_545_575_582-7.patch don't apply anymore.

What should be done about this issue? Can you rebase 582 and 575 and fix the patch?

The #2831739: Allow combined attributes now requires #2755529: Product variant bulk creation. Hence, both patches are merged. So, all that you need to fix the current issue is to apply the combined attributes patch:

https://github.com/drupalcommerce/commerce/compare/8.x-2.x...drugan:allo...

Optionally, if you have issues with displaying multiple Add to Cart forms on a page you can apply this patch:

https://github.com/drupalcommerce/commerce/compare/8.x-2.x...drugan:mult...

It fixes #2827721: AddToCartForm should support all purchasable entities and some of the related issues.

Also, as you now have combined attributes functionality possibly you want attributes to be grouped on the select dropdown list. Read more on the #48 comment.

Do not forget to flush cache after applying patches.

Patch no longer applies.

@JeroenT

As you did not specified which patch you can't to apply, so I've rerolled all the three against the latest commit.

#2755529: Product variant bulk creation
patch: https://github.com/drupalcommerce/commerce/compare/8.x-2.x...drugan:vari...

#2831739: Allow combined attributes
Includes the variation bulk creation patch and required to fix the current issue.
patch: https://github.com/drupalcommerce/commerce/compare/8.x-2.x...drugan:allo...

Multiple Add to Carts on a page. Has multiple closed issues marked as fixed but for some use cases may appear again.
patch: https://github.com/drupalcommerce/commerce/compare/8.x-2.x...drugan:mult...

Hello Drugan,

I have tested the second patch in your last comment (https://github.com/drupalcommerce/commerce/compare/8.x-2.x...drugan:allo...). It works in a way. Thank you. The only thing is now even if an attribute is not used in any of the variations, this attribute select is still visible with: "No options available..". Previously those attributes were hidden on the add to cart form.

Also I am getting a notice when I add an item to cart:

Notice: Undefined offset: 0 in Drupal\commerce_product\Plugin\Field\FieldWidget\ProductVariationAttributesWidget->selectVariationFromUserInputCustom() (line 381 of modules/contrib/commerce/modules/product/src/Plugin/Field/FieldWidget/ProductVariationAttributesWidget.php).

The variation is added to cart correctly nonetheless.

After applying the patch, selected attribute comes to the first position. This causes a reorder in the attributes. I saw that this is purposefully done. But I believe this is incorrect behavior. Especially if you thing about radios. Noone wants the selected option to move when one selects an option. I have never seen such behavior on a website.

@Sinan Erdem

To your #55 question:

Check Hide empty field checkbox.

Read more here: #2730643: Product variation attributes shows combines variations that not exist.

To the #56:

Can't reproduce. Needs more info.

After applying the patch, selected attribute comes to the first position.

As you've noticed it is purposely done to fix exactly the current issue:

#2707721: Incorrect display of attribute field values on the Add To Cart form

Specifically, create attributes/variation type/product type like in the example provided by the issue reporter:

There is not yet found a way to fix this issue without attributes reordering after changing an attribute on the Add to cart form. Any possible solutions are welcome.

Note that the patch is just a temporary solution and will be moved in a contrib module as soon as I have enough free time for this work. There I am going to add an attribute widget config option to reorder or not attributes.

Thank you Drugan. This patch at least gives us a base to work with.

Update: Fixed. Changed product variation form display settings to 'commerce select list' and 'hidden' option selected.

Hi Drugan, I've found an issue. I'm using the combined patch above to fix the problem where the add to cart form would show variations that were not available, working great. I am not using it for combined attributes.

But, with products that have either a) no attributes or b) a single attribute choice - I get the white screen of death and the following errors in the log.

Notice: Undefined offset: 0 in Drupal\commerce_product\Element\CommerceProductRenderedAttribute::processRadios() (line 41 of /home/cpldcouk/cpladmin.com/web/modules/contrib/commerce/modules/product/src/Element/CommerceProductRenderedAttribute.php)

TypeError: Argument 1 passed to Drupal\Core\Entity\EntityViewBuilder::view() must implement interface Drupal\Core\Entity\EntityInterface, null given, called in /home/cpldcouk/cpladmin.com/web/modules/contrib/commerce/modules/product/src/Element/CommerceProductRenderedAttribute.php on line 41 in Drupal\Core\Entity\EntityViewBuilder->view() (line 110 of /home/cpldcouk/cpladmin.com/web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php)

The products that are not working belong to a product type which has an attribute with the element type of "Rendered Attribute" (so we can output swatches). The products that are not working are not using this attribute. If I set this element type to "Select List" the problem goes away.

Paolo.

OK, I've also updated the current comment....

I have the same notice as #60 ... Changed product variation form display settings to 'commerce select list' and 'hidden' option selected. still getting this notice.

@drugan @drupalstrap, same here, the issue is back after applying the patch to latest dev. This is also with changing product variation form settings to 'commerce select list' and 'hidden'. It is also happening to all products, not just those with no or a single attribute. @drugan if you need any more info please do get in touch. I've rolled back the patch meanwhile.

Undefined offset: 0 in Drupal\commerce_product\Element\CommerceProductRenderedAttribute::processRadios() (line 41 of /home/cpldcouk/cpladmin.com/web/modules/contrib/commerce/modules/product/src/Element/CommerceProductRenderedAttribute.php)

OK, I'll check it in the next few days...

Please, revert the patch and apply it again.

https://github.com/drupalcommerce/commerce/compare/8.x-2.x...drugan:allo...

Note that it is recommended to use combined attributes patch if a product has some number of optional attributes. The reason is that the DC does not support - NONE - choice on an attribute though it might be displayed on the Add to Cart form select list. On the radios fieldset, you'll not see even that. Also, use this patch to avoid problems like the one described in the current issue summary when all attributes are required fields but have just a few variations created on a product having a lot of them. See:

https://www.drupal.org/files/issues/_add%20to%20cart.png

Seems that standalone variation bulk creator patch has less sense as it allows to create all product variations but not all of them can be added to the cart because of the above reasons. So, the contrib where I am going to move this functionality will be based on the combined attributes patch. Note that using this patch you are not obligated to actually combine attributes. It's just an option which could be used and it does not interfere in any place of the currently supported attributes handling process.

The latest version of the combined attributes patch now has setting whether to reorder attributes or not when the user makes a choice on an attribute. So, if your attributes work flawlessly without reordering then you can uncheck the Reorder Labels box. See #57.

I had the same problem implementing my website.
The problem occurs when we have product_variation that create a non overlapping product_attribute_value sets:
i.e. we have the following attributes:
1) Attribute A with values [A1, A2, A3]
2) Attribute B with values [B1, B2, B3]
Now whe ave a Product P with the following variations:
1) Pv1 = [A1, B1]
2) Pv2 = [A1, B2]
3) Pv3 = [A2, B2]
The attribute order on the product_variation is A, B.

When we use the "Add to cart form" Format on product type we are falling back to display the Product Variation attributes via the "Product variation attributes" widget defined on the Order Item Type.

The bug occurs when selecting attributes we are going to select a new Product Variation witch has an attribute set not overlapping with the previous one, continuing with the above example, I try to explain with two scenario (firs buggy scenario one, then with the working one):
1) BUGGY SCENARIO
As we get into the Product Display the Product Variation Attribute show the Pv1 [A1, B1].
Here if we change the attribute selection A from A1 to A2, the widget do not works properly selecting the Pv3, but it stay stuck with the Pv1.
This happen because the User selection variation has became [A2, B1] and no product variation exist with these variations. The widget has fault back to Pv1 [A1, B1] simply because is the first product variation of the list. With the selection of Pv1 the widget will display the attribute selection lists generated by Pv1: A => [A1, A2]; B => [B1, B2] and Product Variation Pv1 with the selected attribute [A2, B1] that is not consistent with the data.

2) WORKING SCENARIO
As we get into the Product Display the Product Variation Attribute show the Pv1 [A1, B1].
Here if we change the attribute selection B from B1 to B2, the widget works properly and select the Pv2, this because there widget search the selected variation with the selected attribute [A1, B2] that exist and it's Pv2. In this case the widget work properly and display the attribute selection lists generated by Pv2: A => [A1, A2], B=>[B1, B2] and Product Variation Pv2 with the selected attribute [A1, B2].

I solved the bug changing the method Drupal\commerce_product\Plugin\Field\FieldWidget\ProductVariationAttributesWidget::selectVariationFromUserInput that is going to select the best matching Product Variation from the selected attribute considering the Attribute Selection order.
I.e. with the BUGGY SCENARIO we have that selecting the A2 attribute, the method return Pv3. This because we find the best candidates from the selection of A2, that in our example is only Pv3.

Here attached is the patch I have craeted. I hope this helps.

@AndreaMaggi
Thank you! This scenario is golden for writing a test.

@bojanz
Thanks to you!
I have seen that the patch I have attached is reported as "Patch Failed to Apply". Probably I've done something wrong creating the patch: I created the patch only for the file and not for the project. Could be this the problem? I'm not really used to that. I've always worked alone.
But because the fix works I'm going to paste here the code I have wrote, so probably someone else will be able to create the patch.

I would have liked to create a Test for that, but I'm actually able to create only Kernel test, and usually I have seen widget tested only with functional test.

By the way here the code:

Class: Drupal\commerce_product\Plugin\Field\FieldWidget\ProductVariationAttributesWidget

protected function selectVariationFromUserInput(array $variations, array $user_input) {
    $current_variation = reset($variations);
    $candidate_variation = $variations;
    if (!empty($user_input['attributes'])) {
        $attributes = $user_input['attributes'];
        foreach ($attributes as $field_name => $value){
            $discarded_variation_ids = [];
            foreach ($candidate_variation as $variation){
                if ($variation->getAttributeValueId($field_name) != $value) {
                    $discarded_variation_ids[$variation->id()] = $variation->id();
                }
            }
            $discarded_variation = NULL;
            foreach ($discarded_variation_ids as $discarded_variation_id){
                if ($discarded_variation == NULL){
                    $discarded_variation = $candidate_variation[$discarded_variation_id];
                }
                unset($candidate_variation[$discarded_variation_id]);
            }
            if (empty($candidate_variation)){
                return $discarded_variation;
            }
            if (sizeof($candidate_variation) ==  1){
                return reset($candidate_variation);
            }
      }
    }
    return $current_variation;

The patch worked fine for me, but I needed to add an additional folder level a/ & b/. I'm also not a patch guru and there might be a simpler method, but I usually do it like this (if you're using Linux):

[... Make sure you have the original commerce version without your changes ...]
cd modules
cp -ax commerce commerce.old
[... edit file(s) with your favorite editor or replace them with your new version ...]
diff -ur commerce.old commerce > my_cool_new.patch
rm -rf commerce.old

So, yes. You should create the patch for the commerce folder, not for the file.

Ah, I think I found another problem with the patch:

I have to attributes: "Package" and "Color".
Package can be "5E", "10E" or "5F"
5E is available in many colors: red, blue, green, orange, pink etc. (but no gray!)
10E is available in 2 gray colors: dark gray, light gray
5F is available in: medium gray

So all 3 packages don't share any colors. Now, if I change the package, I'd expect it to only show the relevant set of colors.
At first, when I load the page 5E is selected. Many colors are shown. OK!
Now I select 10E: Only dark gray and light gray are shown. OK!
Now I select 5F: Only medium gray is shown. Select box becomes disabled. OK!
Now I select 10E again: The colors from 5E are shown but of course only the 2 gray colors from 10E should be shown. BUG!

Sorry for the somewhat complex example, but I wanted to stick as much to the original here as possible so you don't have problems reproducing this.

I found that the problem I described, can be solved by replacing "return $current_variation;" at the end of the function with "return reset($candidate_variation);"

Don't know whether this is the correct fix, though :-/

Hello Guys,

I've applied the patch and it is fixing the issue partly.
I've extend it in order to handle multiple attribute fields to be checked, because only checking the value of the previous attribute is not enough, so we should check all previous attributes when building values.

After testing the patch I found some issues with it and I did some refactoring and fixing.
Uploading the latest version of the patch.

Some feedback from my experience with the above to help out.

#73 fails when applying the patch to latest dev (as of 29th Jan). Manually inserting the patch code fixes the issue and attributes display perfectly for my use case. This of course doesn't handle the optional attribute handling per the patch from Drugan #65 which is something we need, but for those that don't, #73 is a handy solution to this issue.

The patch from #65 fails on modules/product/commerce_product.services.yml (10) and just needs tweaking to insert the code for this section in the right place further down. Manually fixing this, the patch then applies perfectly and attributes work just as they do in #65 with the added functionality Drugan provided with the 'commerce select list' formatter and the optional attribute facility being a good example of that which we use.

Discovered I need to learn how to quickly create and fix patches when I have some free time.

This issue is now fixed on the Commerce Correct Attributes submodule of the Commerce Extended Attributes.

Excellent contribution Drugan, this is an essential part of my project. Will be looking at this today and providing feedback on any issues along the way.

I also think that this should go with the Drupal Commerce core and not as a separate module.
I doesn't make sense to have the attributes in core and in order to make them working to install additional module.
IMO @drugan I think you should work on fixing the attributes in the commerce core and having only extended functionality in your module.

@skek @Londova

I was claming the same earlier but unfortunately I have no commit access and should rely on a verdict of @bojanz or @mglaman, lead maintainers of the DC. And partly I understand why they even did not participated in the discussion. One of the reasons is that in some places my code may look ugly from the point of view of a professional Drupal developer. Yes, I agree but my approach to fixing issues is the following: first - create a working code, second - refactor, refactor, refactor... The maintainers' approach is a bit different: first - create perfect code, then we'll look into it.

This is not possible for such complex issues as the current one to create by one developer a solution which will satisfy all the requirements. One of them is to break a solution into pieces (different issues) which could be comfortable to review by the maintainer. This is huge amount of work. Add to these requirements full test coverage for each of the pieces and you'll understand that one developer will never be able to perform such a work. Also, don't forget that all the pieces should be reviewed and commited otherwise the whole work may be easily thrown into the bin as it has no sense without even just one minor piece not being commited. That is why I don't believe that my and similar solutions will ever become commited.

If you've noticed the current versions of the Commerce Bulk, Commerce Extended Attributes and the actual end worker of the solution - the Commerce Correct Attributes, even don't have tests. Which I will write eventually, one by one, as soon as I have time for it. But for now we can rely just on manual testing. However, it is better than to have unfixed bug preventing to use all the power of the Drupal Commerce.

@skek by the way your solution #73 is great as it passed Londova-test and I even scratched my head how to integrate it into the commerce_cattributes, but again: time, time, time... For the same reason I did not test it for wqmeng-test to pass. Would be glad if someone will figure out how to do it as this approach to loop through attributes values looks more elegant and also compact.

When it comes to bug reports, I am actually not asking for a solution at all. All I need is an expanded, failing test. That allows me to attack it with code until it becomes good. Unfortunately, writing a good test is harder than writing initial code. Without a test, there is no guarantee that a refactor will still contain a fix for the same problem.

I estimate that reproducing this issue and writing a test will take me a full day, making it tough to schedule against the many other parallel efforts we have going on, hence it still being open.

OK, someone makes an effort spending their own time for writing a failing test. So, what? Nothing happens - the test nor commited, neither reviewed. Even not discussed by the maintainer. The contributor does not know may be they are wrong somewhere and what to do next. As the result might be disappointment.

I can't believe that you "attack" tests just written by you personally. If that is true then no wonder that the current DC looks for a newcomer as an abandoned project. No solutions for major issues, no hot discussions, just a few of approved contributors. Occasional new issues from newcomers. Most of them are just duplications of the old ones. And again: no solutions from the maintainer who claims that he does not need any help and will do everything by himself. Voua la! Coming in - coming out.

Guys, I think we are over reacting here and we should calm down.
I'm also a contributor to a module and I know how much time it is taking and how hard is it at the end so I really respect every effort.
I was just pointing that the best place to have this changes are in the commerce core, nothing against @drugan module, actually even opposite, great we have a workaround for now.
So thank you to all involved in the issue and to all doing best to make the things working! We are all Drupal users at the end so we should work together to make it great! So, calm down and lets be constructive. Wish you a great day!

I have no idea what happened here. But #66 has a test scenario description. No one has produced a test. My goal is to grok #66 and turn it into a test.

In https://www.drupal.org/project/commerce/issues/2941834#comment-12496762 I realized we have some missing test coverage of what default attributes should be when visiting in a variation's context (?v=)

Just to add my $0.02 - the patch in #2707721-72: Incorrect display of attribute field values on the Add To Cart form fixes the problem for me (same scenario as described in #2707721-66: Incorrect display of attribute field values on the Add To Cart form - two attributes)

Thanks @AndreaMaggi

miiimooo thanks for helping bring some clarity to this issue.

Okay, skek's logic in #72 and #73 are what we need. The main problem were tests. There were no tests. There was always code but no tests to prove validitity.

This splits out the attribute + variation logic into a service so it is testable. This helped me resolve our logic and makes the widget's code less complex. And stable.

One of the reasons is that in some places my code may look ugly from the point of view of a professional Drupal developer. Yes, I agree but my approach to fixing issues is the following: first - create a working code, second - refactor, refactor, refactor... The maintainers' approach is a bit different: first - create perfect code, then we'll look into it.

We do not ask for a perfect piece code. I don't write perfect code. We ask for tested code that is ugly, perfect, or partial. Because the tested code is code which can be refactored. As maintainers, we have many bugs to trace over many issue queues. We cannot manually reproduce each and hope the code works and then lays in rest untested.

No idea why this wouldn't apply.

Apparently modifying the TravisCI YAML caused all the grief.

1. +++ b/modules/product/commerce_product.services.yml
   @@ -16,3 +16,7 @@ services:
   +  commerce_product.variation_attribute_value_resolver:
   +    class: Drupal\commerce_product\ProductVariationAttributeValueResolver
   +    arguments: ['@entity_type.manager', '@entity.repository', '@commerce_product.attribute_field_manager']
   

   Naming things is hard.

2. +++ b/modules/product/src/ProductVariationAttributeValueResolverInterface.php
   @@ -0,0 +1,50 @@
   +interface  ProductVariationAttributeValueResolverInterface {
   ...
   +  public function resolve(array $variations, array $attribute_values = []);
   ...
   +  public function getAttributeInfo(ProductVariationInterface $selected_variation, array $variations);
   ...
   +  public function getAttributeValues(array $variations, $field_name, callable $callback = NULL);
   

   I feel this might need to be two different services.

   getAttributeValues might not be needed, but I exposed it for testing.

   Really just need:

   * getVariationByAttributeValues: What variation should be used out of this group based on known attribute values
   * getAttributesInfobyVariations: from these variations and a selected variation, what attribute information is available.

Ugh, I just realized this patch was meant for https://www.drupal.org/project/commerce/issues/2730643. Either way, I think they're the same issue.

Rename class to ProductVariationAttributeMapper. This better supports the in/out methods. Then phpcs fixes.

Renamed.

+++ b/modules/cart/tests/src/FunctionalJavascript/AddToCartOptionalAttributeTest.php
@@ -0,0 +1,195 @@
+
+
...
+
+
...
+
+

Need to clean up extra spaces.

To aid in manual testing of a fairly complex real product I've written a utility function that outputs a products variants as a string formatted for csv

It's quick and dirty but makes it easy for me to see all the available combinations on my products.

function print_variant_table($productId) {
  $product = Product::load($productId);
  $variants = $product->getVariations();
  $table = [];
  $headers = ['sku'=> ''];

  /** @var ProductVariation $defaultVariation */
  $defaultVariation = $product->getDefaultVariation();
  foreach ($defaultVariation->getAttributeFieldNames() as $attributeFieldName) {
    $headers[$attributeFieldName] = '';
  }

  $table[] = array_keys($headers);
  foreach ($variants as $variant) {
    /** @var ProductVariation $variant */
    $attributes = $variant->getAttributeValues();

    $value = $headers;
    $value['sku'] = $variant->getSku();

    foreach ($attributes as $attributeField => $attribute) {
      $value[$attributeField] = $attribute->getName();
    }

    $table[] = array_values($value);
  }

  $imploded = '';
  foreach ($table as $row) {
    $imploded .= implode(",", $row) . "<br>";
  }

  return $imploded;
}

Accidentally committed this mid-review. Doesn't pass phpcs, so I couldn't keep it.

Progress looks amazing and the extended test coverage in ProductVariationAttributeMapperTest() is extra great.

I wrote the initial functional test 2 year ago. There I introduced the word "Mutually Exclusive Attributes Matrix".
That term is opaque and complex.
And I am not sure that test case is solved now.

Let me explain:

Current implementation

1. Current widget uses dropdowns / selectors for each attribute field.
2. The widget updates the list of attributes that can be choosen from, depending on the current selected product variation
3. The user changes attribute-values until she has the product-variation she wants.

Problem

1. When it is allowed to have 'missing' product-variations for combinations of attributes
2. Then this widget can have an unresolveable path from one product-variation to another.

task: prove this with an example

Solution

1. Have an alternative product-variation selector widget; for example a dropdown with just 'product variations' instead of 'attribute fields'.
2. Make it possible to configure which product-variation selector widget must be used
3. Additionally: extend the widget with an 'unresolvable' state, so it will not be used on runtime

The 'Mutually Exclusive' example:

• This example has 8 product-variations of one type.
• It is quite artificial and a bit silly, but I hope it is understandable.
• It has 2 attributes (color and size).
• It has 2 groups of attribute-combinations; one around babies and one around adults.
• If you start in one of the groups, it is impossible to jump to the other group with the current attribute-widget.
• The reason is that the attribute selector cannot show values from the other group; they are 'Mutually Exclusive'

@ndf

Unfortunately, I see no any progress.

The ideal solution should have and pass three tests:

Londova test (all attributes are required but only a few variations are created)

wqmeng test (at least on attribute is optional and only a few of them are created)

drupalstrap test (all attributes are optional and at least on of them is rendered attribute with support of N/A combination (variation) when no one attribute is chosen)

Any other partial solution sooner or later will return us to the same issue. For example, the Londova test could be easily fixed with the skek's solution (why it was not included in the latest patch?) but making just one of the test attributes optional will lead to a fail. Remember, product attributes combinations (available variations) are not solid things because they dynamically change together with the stock change. You'll never know which comes next. The same with the order in which they appear on a page. Just moving one attribute above the other on a variation type settings form may result in a new entry on the DC issue queue (read the current issue summary).

Unfortunately, I see no any progress.

The ideal solution should have and pass three tests:

Please help write tests (explicit cases, or even code), then, instead of saying we're missing coverage or providing uncommented/untested solutions. We cannot fix these issue by metaphorically throwing spaghetti at the wall to see if it sticks.

I know you have your xattributes module. I tried to even use it as a reference to pick it apart and get proper fixes committed. But it's undocumented, so I have no idea what was changed for what reason.

We're not against fixing this. But we need to commit stable code. Bugs show faults in our assumed stability, which means we need more tests. Otherwise, we're committing more assumptions.

This last patch makes it easier to test use cases and find bugs. The next step I'll be doing is reviewing #104

@ndf: should we even support something like that? it feels like that is two different products at that point.

I'll still be going through and grokking items addressed by drugan in #106.

@mglaman the example from #104 is kind of silly but you could get into the same situation with sizes S,M,L,XL and 4 colors(red, blue, green, yellow) seems like it would be a common scenario for a discontinued product in sell down mode.

An alternative solution to the new widget suggested in #104 is automagicaly creating an 'N/A' option for any attribute that isn't set on all enabled variations. There would some additional complexity to the add to cart form since the add button would have to be disabled until a valid set of attributes was selected but would be a better user experience.

@mglaman

totally agree with you tests are crucial part of any software but as it was noticed here " writing a good test is harder than writing initial code" for me personally the main issue is that test runs in a black box and you'll never know that you did a stupid typo before the test is completed so the whole process is quite time consuming add to this my old laptop's poor capabilities and you'll understand why the commerce_xattributes still have no one test added my plan is the following:

1. Buy a new (super) laptop
2. Make full test coverage for existing code base
3. Add new features to the commerce_xattributes
4. Write tests for the features

As the commerce_xattributes solution for this issue is quite complex and might be seen as arguable so I am not sure if it could be commited into the Drupal Commerce as a whole but if someone will take at least some parts of the solution and try to integrate it then I'd be glad to help but again writing partial solutions whether they covered with tests or not is just waisting of time we should kick this issue in the ass once and forever

Okay, here is an updated patch to test the issue summary of Londova's issue (it wasn't originally addressed because I posted patch from the wrong issue into here.) It's going to fail.

Here is the problem. When the attribute match does not exist, it falls back to the default variation. It seems like some of the agreement here is that the last attribute should be considered "invalid", then and pick the last valid variation. In this test case, it would be the "Medium, Green, 100" option.

    // Current this fails because it is an invalid variation option. It is
    // causing the resolved variation to be the default.
    $resolved_variation = $this->mapper->getVariation($variations, [
      'attribute_size' => $this->sizeAttributes['medium']->id(),
      'attribute_color' => $this->colorAttributes['blue']->id(),
    ]);
    $this->assertEquals($variations[2]->id(), $resolved_variation->id());
    $this->assertEquals('Medium', $resolved_variation->getAttributeValue('attribute_size')->label());
    $this->assertEquals('Green', $resolved_variation->getAttributeValue('attribute_color')->label());
    $this->assertEquals('100', $resolved_variation->getAttributeValue('attribute_pack')->label());

@drugan @mglaman
What makes me happy here is that there are green tests and clean code.
What is missing is a clear statement what use-cases are supported and what not.
If we clearly describe what we do support to site-builders (and include test-coverage for the code part) that would be perfectly fine for me.

drugan in #105 names usecases that are not covered due to optional attributes and/or availability of product-variations. The case I described in #104 is a variation of the Londova test.
We don't have to support all of those use-cases. It can be done in follow-ups and contrib. It would be nice though if those can build upon this issue.

@mglaman
One alternative widget (as described in #104) to select available product variations would be a simple solution for site-builders that have a non-supported use-case. This can be done in a new issue.

Note that I think the 'mutually exclusive' scenario from #104 it is fundamentally unsolvable with the attribute-selector. What I would like to discuss is if the widget should have a fallback on runtime.

So now I will review the patch as-is first
This will take a couple of days though :)
Then I will try to write test #104 and solution and add it as patch and interdiff here!

@ndf @mglaman

I'd recommend the #73 to pass the Londova test I've tested this and it works perfectly if it will be decided to not support optional attributes on the DC then the mutual exclusive and similar tests don't need at all just Londova test which is by chance is a good example on how attributes values could broken

Here is a patch which re-integrates variation resolving logic in #73. This fixes the Londova test and allows "resetting" dependent attributes as the top-down selection changes. I need to test getAttributeInfo still for this change. But I wanted to post the patch so DrupalCI can run FunctionalJavascript tests.

Didin't realize I made a bad patch.

Okay! This tests the options output from the Londova test. The key trick is also the attribute weight from the variation's form display. Documented in the test. Spent a good while on a failing test forgetting to adjust the weight.

These tests now cover

* Londova test (all attributes are required but only a few variations are created)
* wqmeng test (at least on attribute is optional and only a few of them are created)

I think this is a majority use case.

I'd like to visit this one in a follow up:

* drupalstrap test (all attributes are optional and at least on of them is rendered attribute with support of N/A combination (variation) when no one attribute is chosen)

Patch to fix phpcs

1. index 00000000..e5127308
   --- /dev/null
   
   --- /dev/null
   +++ b/modules/cart/tests/src/FunctionalJavascript/AddToCartOptionalAttributeTest.php
   
   +++ b/modules/cart/tests/src/FunctionalJavascript/AddToCartOptionalAttributeTest.php
   @@ -0,0 +1,192 @@
   

   Javascript testing of the widget can be dropped now, all testing is in the service.
   If we keep the test it needs cleanup. It has for example comments that are not in sync with the code.

2. +++ b/modules/product/src/Plugin/Field/FieldWidget/ProductVariationAttributesWidget.php
   @@ -224,24 +236,8 @@ class ProductVariationAttributesWidget extends ProductVariationWidgetBase implem
      protected function selectVariationFromUserInput(array $variations, array $user_input) {
   -    $current_variation = reset($variations);
   ...
   -    return $current_variation;
   +    $attributes = !empty($user_input['attributes']) ? $user_input['attributes'] : [];
   +    return $this->variationAttributeValueMapper->getVariation($variations, $attributes);
   

   Yes!

3. +++ b/modules/product/src/Plugin/Field/FieldWidget/ProductVariationAttributesWidget.php
   @@ -256,48 +252,7 @@ class ProductVariationAttributesWidget extends ProductVariationWidgetBase implem
      protected function getAttributeInfo(ProductVariationInterface $selected_variation, array $variations) {
   -    $attributes = [];
   -    $field_definitions = $this->attributeFieldManager->getFieldDefinitions($selected_variation->bundle());
   ...
   -    return $attributes;
   +    return $this->variationAttributeValueMapper->getAttributeInfo($selected_variation, $variations);
   
   @@ -314,20 +269,7 @@ class ProductVariationAttributesWidget extends ProductVariationWidgetBase implem
      protected function getAttributeValues(array $variations, $field_name, callable $callback = NULL) {
   -    $values = [];
   ...
   -    return $values;
   +    return $this->variationAttributeValueMapper->getAttributeValues($variations, $field_name, $callback);
   

   Another 2 yesses!

4. +++ b/modules/product/src/ProductVariationAttributeMapper.php
   @@ -0,0 +1,171 @@
   +    $current_variation = reset($variations);
   +    if (empty($attribute_values)) {
   +      return $current_variation;
   +    }
   +    foreach ($attribute_values as $attribute_field_name => $attribute_value) {
   +      if (!empty($variations)) {
   +        foreach ($variations as $key => $variation) {
   +          if ($variation->getAttributeValueId($attribute_field_name) != $attribute_value) {
   +            unset($variations[$key]);
   +          }
   +        }
   +        if (!empty($variations)) {
   +          $current_variation = reset($variations);
   +        }
   +        else {
   +          break;
   +        }
   +      }
   +    }
   

   Is it possible to add a comment about the inner working of this function?
   At first sight I thought it would always return the first variation that is passed in.
   Might be a target for a unit test.

5. +++ b/modules/product/src/ProductVariationAttributeMapper.php
   @@ -0,0 +1,171 @@
   +      // The first attribute gets all values. Every next attribute gets only
   +      // the values from variations matching the previous attribute value.
   +      // For 'Color' and 'Size' attributes that means getting the colors of all
   +      // variations, but only the sizes of variations with the selected color.
   

   Can the reason for this be added?
   Believe this is recently introduced in this patch and a major shift and improvement.

6. +++ b/modules/product/tests/src/Kernel/ProductVariationAttributeMapperTest.php
   @@ -0,0 +1,723 @@
   +    $product = $this->generateThreeByTwoScenario();
   ...
   +    $product = $this->generateThreeByTwoScenario();
   ...
   +    $product = $this->generateThreeByTwoScenario();
   

   A bit of a strange name, because in setup() I read things about colors, sizes, disks and ram

7. +++ b/modules/product/tests/src/Kernel/ProductVariationAttributeMapperTest.php
   @@ -0,0 +1,723 @@
   +  /**
   +   * Tests that if two attributes are passed, the proper variation is returned.
   +   */
   +  public function testResolveWithWithTwoAttributes() {
   ...
   +    $resolved_variation = $this->mapper->getVariation($variations, [
   +      'attribute_color' => $this->colorAttributes['blue']->id(),
   +      'attribute_size' => $this->sizeAttributes['large']->id(),
   +    ]);
   +    // An invalid arrangement was passed, fall back to last viable option.
   ...
   +    $resolved_variation = $this->mapper->getVariation($variations, [
   +      'attribute_color' => '',
   +      'attribute_size' => $this->sizeAttributes['large']->id(),
   +    ]);
   +    // A missing attribute was passed for first option.
   

   This test also some code-paths unrelated to the '2 attribute' setup. Probably catched and solved here fisrt :)
   May choose to move those 'edge cases' to a specific test function.

8. +++ b/modules/product/tests/src/Kernel/ProductVariationAttributeMapperTest.php
   @@ -0,0 +1,723 @@
   +    // With no callback, all value should be returned.
   +    $values = $this->mapper->getAttributeValues($variations, 'attribute_color');
   +    $this->assertTrue(in_array($this->colorAttributes['red']->label(), $values));
   +    $this->assertTrue(in_array($this->colorAttributes['blue']->label(), $values));
   +
   +    // With callback, only specified values should be returned.
   +    $values = $this->mapper->getAttributeValues($variations, 'attribute_color', function (ProductVariationInterface $variation) {
   +      return $variation->getAttributeValueId('attribute_color') == $this->colorAttributes['blue']->id();
   +    });
   +    $this->assertTrue(in_array('Blue', $values));
   +    $this->assertFalse(in_array('Red', $values));
   

   Nice

9. +++ b/modules/product/tests/src/Kernel/ProductVariationAttributeMapperTest.php
   @@ -0,0 +1,723 @@
   +  /**
   +   * Tests the getAttributeInfo method.
   +   */
   +  public function testGetAttributeInfo() {
   

   Might add the optional attribute too

10. +++ b/modules/product/tests/src/Kernel/ProductVariationAttributeMapperTest.php
    @@ -0,0 +1,723 @@
    +  /**
    +   * Tests the getAttributeInfo method.
    +   */
    +  public function testGetAttributeInfoOptional() {
    

    Ok here it is

11. +++ b/modules/product/tests/src/Kernel/ProductVariationAttributeMapperTest.php
    @@ -0,0 +1,723 @@
    +  /**
    +   * Generates a three by two scenario.
    +   *
    +   * This generates a product and variations in 3x2 scenario. There are three
    +   * sizes and two colors. Missing one color option.
    +   *
    +   * [ RS, RM, RL ]
    +   * [ BS, BM, X  ]
    +   *
    +   * @return \Drupal\commerce_product\Entity\ProductInterface
    +   *   The product.
    +   */
    +  protected function generateThreeByTwoScenario() {
    ...
    +  /**
    +   * Generates a three by two (optional) secenario.
    +   *
    +   * This generates a product and variations in 3x2 scenario.
    +   *
    +   * https://www.drupal.org/project/commerce/issues/2730643#comment-11216983
    +   *
    +   * [ 8GBx1TB,    X        , X ]
    +   * [    X   , 16GBx1TB    , X ]
    +   * [    X   , 16GBx1TBx1TB, X ]
    +   *
    +   * @return \Drupal\commerce_product\Entity\ProductInterface
    +   *   The product.
    +   */
    +  protected function generateThreeByTwoOptionalScenario() {
    

    These 'generate scenario' functions with their docblock including visual table of attributes are very understandable.

12. +++ b/modules/product/tests/src/Kernel/ProductVariationAttributeMapperTest.php
    @@ -0,0 +1,723 @@
    +    foreach ($attribute_values_matrix as $key => $value) {
    +      $variation = ProductVariation::create([
    +        'type' => 'default',
    +        'sku' => $this->randomMachineName(),
    +        'price' => [
    +          'number' => 999,
    +          'currency_code' => 'USD',
    +        ],
    +        'attribute_ram' => $this->ramAttributes[$value[0]],
    +        'attribute_disk1' => $this->disk1Attributes[$value[1]],
    +        'attribute_disk2' => isset($this->disk2Attributes[$value[2]]) ? $this->disk2Attributes[$value[2]] : NULL,
    +      ]);
    +      $variation->save();
    +      $variations[] = $variation;
    +      $product->addVariation($variation);
    +    }
    +    $product->save();
    

    Should this part be in a helper?

13. +++ b/modules/product/tests/src/Kernel/ProductVariationAttributeMapperTest.php
    @@ -0,0 +1,723 @@
    +    $attribute_values_matrix = [
    +      ['small', 'black', 'one'],
    +      ['small', 'blue', 'twenty'],
    +      ['medium', 'green', 'hundred'],
    +      ['medium', 'red', 'twohundred'],
    +      ['large', 'white', 'hundred'],
    +      ['large', 'yellow', 'twenty'],
    +    ];
    

    The Londova scenario!

Amazing progress @mglaman!
Just read the code, did not install the patch yet.

Just tested #118 on simplytest.me but could not get it working.

In the Londova scenario the add to cart widget showed all options in the first selector. But the second selector did not update when I chose an option that should have updated it (from small to medium).

Also I could not find how to switch the first and second selector.

At last might it be a good thing to write js-errors (to console.log) if the attribute-widget gets an errorness input?

Patch from #117 works for me, but only on product page. On View page it is showing me also impossible variations...

OK, After turning off AJAX in View settings everything works fine, however my Views exposed filters need submition by user to take effect...

OK, After turning off AJAX in View settings everything works fine, however my Views exposed filters need submition by user to take effect...

Thanks for testing! There's another issue which documents problems with Views and AJAX. #2897698: With a view catalog, only the first product variations works I believe

Tested #118 on product page and it works for me as well.

Re-tested #118 on a clean install and it works perfect!
Hereby a video (mp4) of the test-result.

Don't know why my simpletest result in #120 was not successful. So that test can be ignored.

Another screen-capture, this time scenario #104.
Work flawlessly.

As mentioned in #120 I could not manage to change the order of the attribute-selectors. But that should be another issue I guess.

Cool! Three +1st today. I'll work on a reroll to address items in #119. I didn't want to touch it until we had some feedback.

+  /**
+   * Gets the attribute values of a given set of variations.
+   *
+   * @param \Drupal\commerce_product\Entity\ProductVariationInterface[] $variations
+   *   The variations.
+   * @param string $field_name
+   *   The field name of the attribute.
+   * @param callable|null $callback
+   *   An optional callback to use for filtering the list.
+   *
+   * @return array[]
+   *   The attribute values, keyed by attribute ID.
+   */
+  public function getAttributeValues(array $variations, $field_name, callable $callback = NULL);

We said we won't make this method public, since the values are already included in the return value of getAttributeInfo().

Here is an updated patch with review comments.

RE:

Can the reason for this be added?
Believe this is recently introduced in this patch and a major shift and improvement.

That is existing logic.

Late night patch and run. Named it wrong.

@mglaman,

Tried the patch and for my use-case it was working great. One thing I noticed, the AddToCartOptionalAttributeTest was no longer included. Was this intended, or did you remove it by accident?

@ndf,

As mentioned in #120 I could not manage to change the order of the attribute-selectors. But that should be another issue I guess.

The order of the attribute-selectors can be changed on the manage form display of the product variation. For e.g. the default product variation type: /admin/commerce/config/product-variation-types/default/edit/form-display. You can drag-and-drop the order of the attribute-selectors over there.

Tried the patch and for my use-case it was working great. One thing I noticed, the AddToCartOptionalAttributeTest was no longer included. Was this intended, or did you remove it by accident?

It's covered by the kernel tests, now. So it reduces patch size. Thanks for catching that though :)

I'll wrap this up.

+   * The variation attribute value mapper.

Nope. "The product variation attribute mapper."

+   * @param \Drupal\commerce_product\ProductVariationAttributeMapperInterface $variation_attribute_value_mapper
+   *   The variation attribute value resolver.

Same here, both the variable name and the description are wrong.

+    $this->variationAttributeValueMapper = $variation_attribute_value_mapper;

The property name is wrong (variationAttributeValueMapper instead of variationAttributeMapper).

+      $container->get('commerce_product.variation_attribute_value_mapper')

The service ID is wrong (again the extra _value_).

   protected function getAttributeInfo(ProductVariationInterface $selected_variation, array $variations) {
+    return $this->variationAttributeValueMapper->getAttributeInfo($selected_variation, $variations);
   }

Let's remove all protected widget methods that now just wrap the mapper ones. No point in keeping them around.

   protected function getAttributeValues(array $variations, $field_name, callable $callback = NULL) {
+    return $this->variationAttributeValueMapper->getAttributeValues($variations, $field_name, $callback);
   }

And this one doesn't even work ;)

+/**
+ * Provides mapping between variations and attributes.
+ *
+ * This is used when working with variations and their attributes, such as
+ * the 'commerce_product_variation_attributes' widget.
+ */
+interface ProductVariationAttributeMapperInterface {

Let's go with:

/**
 * Provides logic for selecting variations using attributes.
 *
 * @see \Drupal\commerce_product\Plugin\Field\FieldWidget\ProductVariationAttributesWidget
 */

Great naming bikeshed #25:
Let's rename getVariation() to selectVariation() and getAttributeInfo() to prepareAttributes().
Let's introduce a PreparedAttribute value object to use as a return value.

Many cleanups here. Close to being committable, but I want to have another pass through the new tests.

Last patch.

Merged several selectVariation() test cases into one for clarity. Expanded assertions for $attribute->getValues() to actually include the whole expected array.

Rewrote the selectVariations() algorithm to make it clearer, documented it on the interface:

diff --git a/modules/product/src/ProductVariationAttributeMapper.php b/modules/product/src/ProductVariationAttributeMapper.php
index 71838eb8..95896c99 100644
--- a/modules/product/src/ProductVariationAttributeMapper.php
+++ b/modules/product/src/ProductVariationAttributeMapper.php
@@ -49,32 +49,26 @@ class ProductVariationAttributeMapper implements ProductVariationAttributeMapper
    * {@inheritdoc}
    */
   public function selectVariation(array $variations, array $attribute_values = []) {
-    $current_variation = reset($variations);
-    if (empty($attribute_values)) {
-      return $current_variation;
-    }
-    // Loop through the provided attribute values and begin to remove variations
-    // which do not match the expected attribute values.
-    foreach ($attribute_values as $field_name => $attribute_value_id) {
-      foreach ($variations as $key => $variation) {
-        // If the variation does not have the attribute value, remove it from
-        // the available candidate variations.
-        if ($variation->getAttributeValueId($field_name) != $attribute_value_id) {
-          unset($variations[$key]);
+    $selected_variation = reset($variations);
+    // Select the first variation that matches the most attribute values.
+    // Start with all attribute values, reduce them by 1 until a match is found.
+    while (!empty($attribute_values)) {
+      foreach ($variations as $variation) {
+        $match = TRUE;
+        foreach ($attribute_values as $field_name => $attribute_value_id) {
+          if ($variation->getAttributeValueId($field_name) != $attribute_value_id) {
+            $match = FALSE;
+          }
+        }
+        if ($match) {
+          $selected_variation = $variation;
+          break 2;
         }
       }
-      // If we still have candidate variations, default to the first one.
-      if (!empty($variations)) {
-        $current_variation = reset($variations);
-      }
-      // If we have run out of candidate variations, break and use the last
-      // selected one to be the current variation.
-      else {
-        break;
-      }
+      array_pop($attribute_values);
     }

-    return $current_variation;
+    return $selected_variation;
   }

   /**

diff --git a/modules/product/src/ProductVariationAttributeMapperInterface.php b/modules/product/src/ProductVariationAttributeMapperInterface.php
index f195fc7a..db410dad 100644
--- a/modules/product/src/ProductVariationAttributeMapperInterface.php
+++ b/modules/product/src/ProductVariationAttributeMapperInterface.php
@@ -14,13 +14,20 @@ interface ProductVariationAttributeMapperInterface {
   /**
    * Selects the best matching variation for the given attribute values.
    *
+   * Takes the first variation that matches the most attribute values.
+   * Partial matches are considered when a full match cannot be made.
+   * For example, when given [Red, Small, Cotton], the search priority is:
+   * 1) [Red, Small, Cotton]
+   * 2) [Red, Small]
+   * 3) [Red]
+   * If no match could be made, the default variation is selected instead.
+   *
    * @param \Drupal\commerce_product\Entity\ProductVariationInterface[] $variations
    *   The variations.
    * @param array $attribute_values
    *   Attribute value IDs, keyed by the field name.
    *
    * @return \Drupal\commerce_product\Entity\ProductVariationInterface
-   *   The matching variation.
+  *   The selected variation.
    */
   public function selectVariation(array $variations, array $attribute_values = []);

Committed. Thanks, everyone.

Opened three followups:
- #2956038: Figure out what to do when variations&attributes are mutually exclusive
- #2956035: Improve support for optional rendered attributes
- #2956043: ProductVariationAttributeMapper::selectVariation() returns the first variation, not the default one
See you there!