We have a product/$product_id url, no reason not to have product/$product_id/$variation_id url. It can even preselect the variation on the add to cart form, which is a long time feature request.

CommentFileSizeAuthor
#23 add_a_canonical_link-2674888-23.patch15.46 KBborisson_
#23 interdiff.txt3.51 KBborisson_
#21 add_a_canonical_link-2674888-21.patch13.03 KBharings_rob
#20 add_a_canonical_link-2674888-1.patch11.9 KBharings_rob
#18 add_a_canonical_link-2674888-18.patch13.15 KBmglaman
#17 add_a_canonical_link-2674888-17.patch9.57 KBmglaman

Comments

bojanz created an issue. See original summary.

harings_rob’s picture

harings_rob commented
Assigned: Unassigned » harings_rob
rszrama’s picture

rszrama commented

I'm hesitant to make the variation URL part of the path like that as opposed to using some other mechanism, like a fragment or query parameter. The reason is that if we add a canonical link, then I believe it would be better to make it part of the URL that is updated as the form is changed. If it's part of the path, then it would be quite difficult to work around path aliases as opposed to using an element of the URL that can change without requiring us to communicate all potential path aliases between the server and client.

bojanz’s picture

bojanz commented

The reason is that if we add a canonical link, then I believe it would be better to make it part of the URL that is updated as the form is changed.

That's a good point. Let's experiment with a ?variation_id then, the routing system should support it.

harings_rob’s picture

harings_rob commented

Ok, so for now we'll start with

variation_id=id

And we'll alter the add to cart form with this data.

rszrama’s picture

rszrama commented

Maybe even something simpler / a bit more ambiguous to end users, e.g. dv=## for default variation?

harings_rob’s picture

harings_rob commented
Status: Active » Needs review

We could add validation on the form part, but it might cause additional load we don't want?

Let me know, so if wanted, I can update it.

https://github.com/drupalcommerce/commerce/pull/349

harings_rob’s picture

harings_rob commented
Status: Needs review » Needs work

Back to needs work as we are not yet sure about the implementation.

harings_rob’s picture

harings_rob commented
Status: Needs work » Needs review

https://github.com/drupalcommerce/commerce/pull/354

New pr.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Needs work

Reviewed the PR.

$query_string_id = \Drupal::request()->query->get('dv')) {

We should inject the request, if not available already.

Also -- should we move dv to be a const on the ProductVariation entity class? So ProductVariation::CANONICAL_QUERY_PARAM or something?

harings_rob’s picture

harings_rob commented

Hi Matt,

Thank you for reviewing, what do you mean by injecting?

Regards,

agoradesign’s picture

agoradesign commented

Hi Rob,

I've opened a PR from your PR, where I've implemented the recommendations from Matt (https://github.com/haringsrob/commerce/pull/1). All changes SHOULD work, I didn't have time to run the code.

Your PR is not in sync with the master branch at the moment - also didn't have time to sync your changes with master before. However it should be a better help for you rather than only writing a comment...

I've put the CANONICAL_QUERY_PARAM constant into the ProductVariationInterface. However I'm not happy with that either. This is cleaner than just the string and makes it easier to replace, however it's still unflexible. What do you think about adding a (static) function, that returns 'dv' just for now. So there won't be an API change, when e.g. a configuration will be introduced in future, or an alter hook...

harings_rob’s picture

harings_rob commented

Reviewing the changes now.

harings_rob’s picture

harings_rob commented
Status: Needs work » Needs review

If tests pass its ready for a second round.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Assigned: harings_rob » mglaman
Status: Needs review » Needs work

This needs some work and updates considering changes since it first was written. Taking this over to finish it up :)

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

harings_rob made the updates, however we need tests for this.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Assigned: mglaman » Unassigned
Status: Needs work » Needs review
StatusFileSize
new add_a_canonical_link-2674888-17.patch9.57 KB

We now have a patch and test!

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
StatusFileSize
new add_a_canonical_link-2674888-18.patch13.15 KB

Update patch to add cart test. Also discovered injected fields were not rendering properly, that is now fixed.

bojanz’s picture

bojanz commented
Status: Needs review » Needs work

Patch no longer applies. Needs a rebase.

   /**
+   * Query string parameter for defining a canonical URL to a product variation.
+   */
+  const CANONICAL_QUERY_PARAM = 'v';
+

This is now unused and should be removed.

+      // Set our variation id as query string.
+      $options = [
+        'query' => [
+          'v' => $this->id(),
+        ],
+      ];
+
+      // Build our url.
+      $route_parameters = $this->getProduct()->urlRouteParameters('canonical');
+      $route_name = 'entity.commerce_product.canonical';
+      $uri = new Url($route_name, $route_parameters, $options);

Nitpick, we build $options, then $route_parameters, then $route_name, but use them in the opposite direction. I find it more readable to build them in the same order in which they are used.

+    // Check if a variation was specified via its canonical URL.
+    // @todo: Core has same problems in regard to translation, follow core's pattern.
+    if ($variation_id = \Drupal::requestStack()->getCurrentRequest()->query->get('v')) {
+      $variation = ProductVariation::load($variation_id);
+    }
+    else {
+      $variation = $entity->getDefaultVariation();
+    }

We are missing the "fallback to default if the referenced variation is invalid (doesn't exist, doesn't belong to the product)" logic that the cart widget has.
Probably the right time to pull this into a service method: ProductVariationStorage::getVariationFromContext($product);

harings_rob’s picture

harings_rob commented
StatusFileSize
new add_a_canonical_link-2674888-1.patch11.9 KB

I took care of the first 2 remarks.

About the Service, this is yet to be created correct?

harings_rob’s picture

harings_rob commented
Status: Needs work » Needs review
StatusFileSize
new add_a_canonical_link-2674888-21.patch13.03 KB

Here is the full patch.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented 
  1. +++ b/modules/product/src/Plugin/Field/FieldWidget/ProductVariationAttributesWidget.php
@@ -247,6 +265,30 @@ class ProductVariationAttributesWidget extends WidgetBase implements ContainerFa
+    if ($query_string_id = $this->requestStack->getCurrentRequest()->query->get('v')) {

    We should use the storage method below. Since it's been loaded it'll be retrieved from memory cache anyways. Reduces madness. Could even let us remove this method. We just run the storage method, then the input one. 

  2. +++ b/modules/product/src/ProductVariationStorage.php
@@ -40,4 +41,23 @@ class ProductVariationStorage extends CommerceContentEntityStorage implements Pr
+    if ($variation_id = \Drupal::requestStack()->getCurrentRequest()->query->get('v')) {

    Since this is not a static function, we should inject this, yeah?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
StatusFileSize
new interdiff.txt3.51 KB
new add_a_canonical_link-2674888-23.patch15.46 KB

Resolves #22.2, not sure what to do about .1

  • bojanz committed b5cc4b2 on 8.x-2.x authored by harings_rob 
    Issue #2674888 by harings_rob, mglaman, borisson_, bojanz, rszrama,...
bojanz’s picture

bojanz commented
Status: Needs review » Fixed

Tweaked, finished, committed. Thanks, everyone.

Status: Fixed » Closed (fixed)

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

alexandersluiter’s picture

alexandersluiter commented

Is there a way to extend this to the SKU? Just need someone to point me in the right direction? Also, some javascript to update the URL parameter without reloading the page (window.history.pushState)?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

@alexandersluiter: please open a new issue instead of reviving an issue that has been closed for over 2 years.

filipetakanap’s picture

filipetakanap commented

How do you activate it... elementary watson?