When using the product variations formatter "Rendered Entity" (entity_reference_entity_view) in the product type display configuration (/admin/commerce/config/product-types/TYPE/edit/display), the rendered entity does not have an 'is active' indicator in the Twig template (commerce-product-variation.html.twig).

Our current workaround is to use THEME_preprocess_commerce_product_variation() to get the variation id from the 'v' URL parameter and check it against the variation id, plus get the default_variation from the product and use it as fallback, if the 'v' parameter is not set.

Issue fork commerce-3553792

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

thomas.frobieter created an issue. See original summary.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Title: Add active marker to variation entities (entity_reference_entity_view) » Add active flag to variation entities (entity_reference_entity_view)

grevil made their first commit to this issue’s fork.

grevil opened merge request !549

grevil’s picture

grevil commented
Version: 3.2.0 » 3.x-dev
Status: Active » Needs review

Alright, this does the trick.

I added the cache context to the variation entity class itself, to be in line with the "Product" entity cache context:

  public function getCacheContexts() {
    return Cache::mergeContexts(parent::getCacheContexts(), ['url.query_args:v', 'store']);
  }

(modules/product/src/Entity/Product.php, line 293-295)

I'd say this won't introduce any issues, since the context is only valid, if the parameter exists in the url. But if one of the maintainers sees problems with this approach, we need to manually set the cache context in the template_preprocessing function or use an alternative approach all together.

Anyway, please review!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

Thanks. Now the active marker class still needs to be set on the variation, right?

grevil’s picture

grevil commented

Sure, if we want that on the default template 🤷‍♂️

grevil’s picture

grevil commented
Status: Needs work » Needs review

Done. Please review.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

There is no class added by default: https://git.drupalcode.org/project/commerce/-/blob/3.x/modules/product/t...

So this formatter class does not make sense.
Adding proper classes is not part of this issue.

Lets add it to the docblock instead.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Assigned: Unassigned » thomas.frobieter
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@thomas.frobieter: FYI: Regularly a select or radio is used to choose the variation and that one has the "selected" functionality. So maybe things should be handled differently, when the rendered entity is used?

I think there should be some kind of "selected" indicator on the selected entity in this case without having to modify templates?
Maybe that means a different (inheriting or wrapping) template should be used?

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented
Status: Needs review » Reviewed & tested by the community

Done. It's still not perfect without the default classes, but, as I said, that's outside the scope of this issue IMO.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

@thomas.frobieter: FYI: Regularly a select or radio is used to choose the variation and that one has the "selected" functionality. So maybe things should be handled differently, when the rendered entity is used?

I think there should be some kind of "selected" indicator on the selected entity in this case without having to modify templates?
Maybe that means a different (inheriting or wrapping) template should be used?

I agree, but I don't think its up to us to invent new classes on this template. Furthermore "selected" is a technical property, which simply is required to make the select work corretly.

I'm not sure where else this template might be used. The file name tells me that it doesn't just render “Swatches.”

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: thomas.frobieter » Unassigned

I'm not sure where else this template might be used. The file name tells me that it doesn't just render “Swatches.”

Correct. That's what I mean by

Maybe that means a different (inheriting or wrapping) template should be used?

.

So let's keep this RTBC'd as a very helpful first step for custom solutions and wait for maintainer feedback. Then in a follow-up the built-in solution should be implemented.

jsacksick’s picture

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

"is_active" should be sufficient considering we're already in the context of variations.
The MR needs a reroll now that we've converted our hooks to OOP hooks.

Also, wondering what is best between calling loadFromContext or comparing the ?v query parameter, maybe calling loadFromContext() makes more sense so we don't have to duplicate the logic.

Also, we aren't really using this right? It's just there for peoplet o use?

khiminrm made their first commit to this issue’s fork.

jsacksick’s picture

jsacksick commented

I renamed the boolean to just "is_active". Could you confirm that still works for you @grevil, @anybody?

jsacksick’s picture

jsacksick commented
Status: Needs work » Needs review

jsacksick’s picture

jsacksick commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thank you @jsacksick!!

grevil’s picture

grevil commented

Hey Jonathan!

Just tested it locally with the new changes. Still works as expected!

Thanks for taking the time! 👍

jsacksick’s picture

jsacksick commented

Great, thanks for the feedback!

Status: Fixed » Closed (fixed)

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