Skip to content
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

Use the admin path name variable to allow the prefix value to be changed #196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

delyriand
Copy link

Ref #117

and see the comment in #118 (comment)

@delyriand delyriand requested a review from a team as a code owner May 16, 2023 07:41
@delyriand
Copy link
Author

Can I have a review? @GSadee or @diimpp?

Copy link
Member

@diimpp diimpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @delyriand, thank you for your contribution. 🙏

Since optimal solution is unavailable due BC promise and flex recipe, both Resources/config/app/routing.yml and Resources/config/app/ajax.yml files should be left where they are, but content can be changed.

So what do you say changing it like so,

# config/app/routing.yml
sylius_admin_order_creation:
    resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml"
    prefix: "/%sylius_admin.path_name%"

sylius_admin_order_creation_ajax:
    resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/ajax.yml"

and

# config/app/ajax.yml
sylius_admin_order_creation_admin_ajax:
    resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin/ajax.yml"
    prefix: "/%sylius_admin.path_name%/ajax"

Yeah, second sylius_admin_order_creation_admin_ajax declaration forces to add admin_ajax to make it unique, which is correct overall name, but doesn't match existing ajax route declaration, but shouldn't introduce any issues and acceptable compromise for me.

Keeping routes at config/app/routing instead of config/routing is acceptable since main file config/app/routing.yml already there and cannot be moved.

@@ -52,7 +52,7 @@ sylius_admin_order_creation_ajax_product_variant_by_codes:
arguments: $code

sylius_admin_order_creation_ajax_provide_available_shipping_methods:
path: /admin/orders/available-shipping-methods/{customerId}/{channelCode}/{shipmentNumber}
path: /orders/available-shipping-methods/{customerId}/{channelCode}/{shipmentNumber}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is correct change since route before this was /admin/ajax/admin/..., it's a sort of BC and should be avoided if possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming this file, which could have been imported directly before is a sort of BC.

Comment on lines +3 to +4
prefix: "/%sylius_admin.path_name%"
resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
prefix: "/%sylius_admin.path_name%"
resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml"
resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin.yml"
prefix: "/%sylius_admin.path_name%"

path: /admin/orders/new/select-customer
methods: [GET]
defaults:
_controller: Sylius\AdminOrderCreationPlugin\Controller\SelectNewOrderCustomerAction

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Comment on lines +94 to +95
prefix: "/ajax"
resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin_ajax.yml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
prefix: "/ajax"
resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin_ajax.yml"
resource: "@SyliusAdminOrderCreationPlugin/Resources/config/app/routing/admin_ajax.yml"
prefix: /ajax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants