Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#23 | add_a_canonical_link-2674888-23.patch | 15.46 KB | borisson_ |
Comments
Comment #2
harings_rob CreditAttribution: harings_rob at Harings.be commentedComment #3
rszrama CreditAttribution: rszrama at Centarro commentedI'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.
Comment #4
bojanz CreditAttribution: bojanz at Centarro commentedThat's a good point. Let's experiment with a ?variation_id then, the routing system should support it.
Comment #5
harings_rob CreditAttribution: harings_rob at Harings.be commentedOk, so for now we'll start with
variation_id=id
And we'll alter the add to cart form with this data.
Comment #6
rszrama CreditAttribution: rszrama at Centarro commentedMaybe even something simpler / a bit more ambiguous to end users, e.g. dv=## for default variation?
Comment #7
harings_rob CreditAttribution: harings_rob at Harings.be commentedWe 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
Comment #8
harings_rob CreditAttribution: harings_rob at Harings.be commentedBack to needs work as we are not yet sure about the implementation.
Comment #9
harings_rob CreditAttribution: harings_rob at Harings.be commentedhttps://github.com/drupalcommerce/commerce/pull/354
New pr.
Comment #10
mglamanReviewed the PR.
We should inject the request, if not available already.
Also -- should we move
dv
to be a const on the ProductVariation entity class? SoProductVariation::CANONICAL_QUERY_PARAM
or something?Comment #11
harings_rob CreditAttribution: harings_rob at Harings.be commentedHi Matt,
Thank you for reviewing, what do you mean by injecting?
Regards,
Comment #12
agoradesign CreditAttribution: agoradesign commentedHi 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...
Comment #13
harings_rob CreditAttribution: harings_rob commentedReviewing the changes now.
Comment #14
harings_rob CreditAttribution: harings_rob commentedIf tests pass its ready for a second round.
Comment #15
mglamanThis needs some work and updates considering changes since it first was written. Taking this over to finish it up :)
Comment #16
mglamanharings_rob made the updates, however we need tests for this.
Comment #17
mglamanWe now have a patch and test!
Comment #18
mglamanUpdate patch to add cart test. Also discovered injected fields were not rendering properly, that is now fixed.
Comment #19
bojanz CreditAttribution: bojanz at Centarro commentedPatch no longer applies. Needs a rebase.
This is now unused and should be removed.
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.
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);
Comment #20
harings_rob CreditAttribution: harings_rob commentedI took care of the first 2 remarks.
About the Service, this is yet to be created correct?
Comment #21
harings_rob CreditAttribution: harings_rob commentedHere is the full patch.
Comment #22
mglamanWe 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.
Since this is not a static function, we should inject this, yeah?
Comment #23
borisson_Resolves #22.2, not sure what to do about .1
Comment #25
bojanz CreditAttribution: bojanz at Centarro commentedTweaked, finished, committed. Thanks, everyone.
Comment #27
alexandersluiter CreditAttribution: alexandersluiter commentedIs 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)?
Comment #28
borisson_@alexandersluiter: please open a new issue instead of reviving an issue that has been closed for over 2 years.
Comment #29
filipetakanap CreditAttribution: filipetakanap commentedHow do you activate it... elementary watson?