-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unify product routing #420
Conversation
taxonSlug: .+ | ||
|
||
sylius_shop_api_product_show_reviews_by_slug: | ||
path: /product/by-slug/{slug}/reviews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to cause a problem. See the related issue Sylius/Sylius#10293 (comment)
_controller: sylius.shop_api_plugin.controller.product.show_product_details_by_code_action | ||
|
||
sylius_shop_api_product_show_catalog_by_code: | ||
path: /taxon-products/by-code/{code} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code
-> taxonCode
?
Now, it is not consistent with routing by slug, and also with you change in UPGRADE
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now, there are taxon-products/by-code/{code}
and taxon-products/by-slug/{taxonSlug}
routings, so they are still not consistent. Sorry for being annoying, it is of course not very important 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to get it done during the day.
@@ -1,23 +1,48 @@ | |||
sylius_shop_api_product_show_details_by_code: | |||
sylius_shop_api_product_show_details_by_code_deprecated: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we are allowed to do bc-breaks, you have documented change and updated docs, I think we can just remove this bc layer. It is not even tested right now and such an upgrade should be relatively easy
Thank you, @mamazu! 🥇 |
Closes #185