Describe your bug or feature request.

Drupal commerce allows the variation associated with a loaded product to be specified by "sku" or "v" query string values thanks to ProductVariationStorage::loadFromContext. However, the only variation tokens that are available with a product require you to use the field delta [commerce_product:variations:0]. It would be nice to access the currently selected variation using token [commerce_product:commerce_product_variation] so that it matches the following scenario:

Product id 1 has variations 1, 2 and 3. When hitting /product/1?v=2 the token [commerce_product:commerce_product_variation:id] would return 2. If there were no argument in the url, the returned id would be 1 (the default)

CommentFileSizeAuthor
#3 0001-Issue-3249743-Add-token-support-for-product-variatio.patch5.86 KBandyg5000

Issue fork commerce-3249743

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

andyg5000 created an issue. See original summary.

andyg5000 opened merge request !78

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented
Status: Active » Needs review
StatusFileSize
new 0001-Issue-3249743-Add-token-support-for-product-variatio.patch5.86 KB

MR ready for review and associated patch attached .

Status: Needs review » Needs work

The last submitted patch, 3: 0001-Issue-3249743-Add-token-support-for-product-variatio.patch, failed testing. View results

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented
Status: Needs work » Needs review
karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented
Status: Needs review » Reviewed & tested by the community

Works great!

jsacksick’s picture

jsacksick commented

The token label says "Default variation", but it isn't actually the default variation that we're returning so it could be misleading I guess.

Here's the comment for the loadFromContext() method.

* Uses the variation specified in the URL (?v=) if it's active and
   * belongs to the current product.
   *
   * Note: The returned variation is not guaranteed to be enabled, the caller
   * needs to check it against the list from loadEnabled().

By reading this, I'd expect the default variation to be returned, (i.e $product->getDefaultVariation());

Also, the merge request needs to be rebased so the pipelines are running.

jsacksick’s picture

jsacksick commented

Perhaps we should call the token "current_variation"? Thoughts?

rszrama’s picture

rszrama commented
Status: Reviewed & tested by the community » Needs work

I think current_variation works, and maybe default_variation is an additional token we should consider supporting. 🧐

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented
Status: Needs work » Needs review

Rebased issue branch, addressed some comments, renamed commerce_product_variation token to current_variation, added default_variation token.

  • jsacksick committed a1630e18 on 8.x-2.x authored by andyg5000 
    Issue #3249743 by KarlShea, andyg5000, jsacksick, rszrama: Add token...

  • jsacksick committed 3b3fa875 on 3.0.x authored by andyg5000 
    Issue #3249743 by KarlShea, andyg5000, jsacksick, rszrama: Add token...
jsacksick’s picture

jsacksick commented
Status: Needs review » Fixed

Updated the array keys per my comments and merged this, thanks everyone!

jsacksick’s picture

jsacksick commented

Oh I suck! I misread the code... It has to be "commerce_product_variation". Reverting my change :)!

  • jsacksick committed 9476dbcb on 8.x-2.x 
    Issue #3249743 followup: Fix the bug introduced.

  • jsacksick committed 8639ee23 on 3.0.x 
    Issue #3249743 followup: Fix the bug introduced.
karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

If you look at my last commit I made exactly the same mistake except in the other argument lol

Status: Fixed » Closed (fixed)

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