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

MenuItemMatcher doMarkSelectedPrettyUrlsMenuItem is broken #6564

Closed
atillay opened this issue Nov 23, 2024 · 1 comment · Fixed by #6568
Closed

MenuItemMatcher doMarkSelectedPrettyUrlsMenuItem is broken #6564

atillay opened this issue Nov 23, 2024 · 1 comment · Fixed by #6568

Comments

@atillay
Copy link

atillay commented Nov 23, 2024

Describe the bug

There is an issue when computing the selected menu items for create and edit urls with the new Pretty urls system (for example : /admin/foo/{id}/edit

See :

$currentRequestUriWithoutQueryString = parse_url($currentRequestUri, \PHP_URL_PATH);
// remove the last segment of the URL but keep the trailing slash (e.g. /admin/post/new -> /admin/post/)
$currentRequestUriWithoutAction = preg_replace('#/[^/]+$#', '', $currentRequestUriWithoutQueryString);
// the edit URL is '/admin/foo/{id}/edit' so we need to remove the two last segments of the URL
if (str_ends_with($currentRequestUriWithoutQueryString, '/edit')) {
$currentRequestUriWithoutAction = preg_replace('#/[^/]+$#', '', $currentRequestUriWithoutAction);
}
$currentRequestUriWithoutAction .= '/';

On my side when I remove $currentRequestUriWithoutAction .= '/'; it fixes the issue.
So I don't really understand why we are adding this trailing slash, maybe my router is not configured in the same way and rewrites url to alway remove trailing slash.

I think the safest way to handle both cases should be to always remove or append the trailing slash on compared values.

To Reproduce

  • Create an entity and the corresponding crud controller
  • Create a dashboard controller and add the linkToCrud to configureMenuItems
  • Visite the create or edit page of an entity item

(OPTIONAL) Additional context
Thanks in advance !

@javiereguiluz
Copy link
Collaborator

Thanks for reporting this bug. In #6568 I'm solving the issue applying your solution.

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

Successfully merging a pull request may close this issue.

2 participants