-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
bug #6383 Update the logic of the menu item matcher (javiereguiluz)
This PR was squashed before being merged into the 4.x branch. Discussion ---------- Update the logic of the menu item matcher Fixes #6147 and fixes #6366 This PR changes the menu item matcher logic completely. Instead of using methods that look at each menu item individually to decide if it's selected or not ... now we have methods that take all menu items at once and find which one should be selected. This is needed to fix edge-cases. I'll add more tests and comments/explanations in the code ... but this can be tested already in apps. I tested in my own apps and everything works as before. The new code feels as fast as before, but I also profiled it with Blackfire to double check it. Commits ------- ee2c0ae Update the logic of the menu item matcher
- Loading branch information
Showing
6 changed files
with
425 additions
and
118 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,19 +3,21 @@ | |
| namespace EasyCorp\Bundle\EasyAdminBundle\Contracts\Menu; | ||
|
|
||
| use EasyCorp\Bundle\EasyAdminBundle\Dto\MenuItemDto; | ||
| use Symfony\Component\HttpFoundation\Request; | ||
|
|
||
| /** | ||
| * @author Javier Eguiluz <[email protected]> | ||
| */ | ||
| interface MenuItemMatcherInterface | ||
| { | ||
| /** | ||
| * @return bool Returns true when this menu item is the selected one | ||
| * Given the full list of menu items and the current request, this method | ||
| * must find the currently selected item (if any) and update its status | ||
| * to mark it as selected. | ||
| * | ||
| * @param MenuItemDto[] $menuItems | ||
| * | ||
| * @return MenuItemDto[] | ||
| */ | ||
| public function isSelected(MenuItemDto $menuItemDto): bool; | ||
|
|
||
| /** | ||
| * @return bool Returns true when any of the subitems of the menu item is selected | ||
| */ | ||
| public function isExpanded(MenuItemDto $menuItemDto): bool; | ||
| public function markSelectedMenuItem(array $menuItems, Request $request): array; | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,82 +6,252 @@ | |
| use EasyCorp\Bundle\EasyAdminBundle\Config\Option\EA; | ||
| use EasyCorp\Bundle\EasyAdminBundle\Contracts\Menu\MenuItemMatcherInterface; | ||
| use EasyCorp\Bundle\EasyAdminBundle\Dto\MenuItemDto; | ||
| use EasyCorp\Bundle\EasyAdminBundle\Provider\AdminContextProvider; | ||
| use Symfony\Component\HttpFoundation\Request; | ||
|
|
||
| /** | ||
| * @author Javier Eguiluz <[email protected]> | ||
| */ | ||
| class MenuItemMatcher implements MenuItemMatcherInterface | ||
| { | ||
| public function __construct(private AdminContextProvider $adminContextProvider) | ||
| /** | ||
| * Given the full list of menu items, this method finds which item should be | ||
| * marked as 'selected' based on the current page being visited by the user. | ||
| * If the selected item is a submenu item, it also marks the parent menu item | ||
| * as 'expanded'. | ||
| * | ||
| * It returns the full list of menu items, including the updated item(s) marked | ||
| * as selected/expanded. | ||
| * | ||
| * @param MenuItemDto[] $menuItems | ||
| * | ||
| * @return MenuItemDto[] | ||
| */ | ||
| public function markSelectedMenuItem(array $menuItems, Request $request): array | ||
| { | ||
| $menuItems = $this->doMarkSelectedMenuItem($menuItems, $request); | ||
| $menuItems = $this->doMarkExpandedMenuItem($menuItems); | ||
|
|
||
| return $menuItems; | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated because you can't decide which menu item to select by only looking at the menu item itself. You need to check all menu items at the same time to solve edge-cases like multiple menu items linking to the same action of the same entity | ||
| * @see markSelectedMenuItem() | ||
| */ | ||
| public function isSelected(MenuItemDto $menuItemDto): bool | ||
| { | ||
| $adminContext = $this->adminContextProvider->getContext(); | ||
| if (null === $adminContext || $menuItemDto->isMenuSection()) { | ||
| return false; | ||
| } | ||
| @trigger_deprecation('easycorp/easyadmin-bundle', '4.11', 'The "%s()" method is deprecated. Use the "%s()" method instead.', __METHOD__, 'markSelectedMenuItem()'); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated because you can't decide which menu item to render expanded by only looking at the menu item itself. You need to check all menu items at the same time. | ||
| * @see markExpandedMenuItem() | ||
| */ | ||
| public function isExpanded(MenuItemDto $menuItemDto): bool | ||
| { | ||
| @trigger_deprecation('easycorp/easyadmin-bundle', '4.11', 'The "%s()" method is deprecated. Use the "%s()" method instead.', __METHOD__, 'markExpandedMenuItem()'); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * @param MenuItemDto[] $menuItems | ||
| * | ||
| * @return MenuItemDto[] | ||
| */ | ||
| private function doMarkSelectedMenuItem(array $menuItems, Request $request): array | ||
| { | ||
| // the menu-item matching is a 2-phase process: | ||
| // 1) scan all menu items to list which controllers and actions are linked from the menu; | ||
| // this is needed because e.g. sometimes we match a menu item that doesn't have the exact | ||
| // action but links to the INDEX action of the same controller of the current request | ||
| // 2) decide which is the most appropriate menu item (if any) to mark as selected based on the current request | ||
| $controllersAndActionsLinkedInTheMenu = $this->getControllersAndActionsLinkedInTheMenu($menuItems); | ||
| $currentPageQueryParameters = $request->query->all(); | ||
| $currentRequestUri = $request->getUri(); | ||
|
|
||
| foreach ($menuItems as $menuItemDto) { | ||
| if ($menuItemDto->isMenuSection()) { | ||
| continue; | ||
| } | ||
|
|
||
| if ([] !== $subItems = $menuItemDto->getSubItems()) { | ||
| $menuItemDto->setSubItems($this->doMarkSelectedMenuItem($subItems, $request)); | ||
| } | ||
|
|
||
| $menuItemQueryString = null === $menuItemDto->getLinkUrl() ? null : parse_url($menuItemDto->getLinkUrl(), \PHP_URL_QUERY); | ||
|
|
||
| $menuItemQueryParameters = []; | ||
| if (\is_string($menuItemQueryString)) { | ||
| parse_str($menuItemQueryString, $menuItemQueryParameters); | ||
| } | ||
|
|
||
| if ([] === $menuItemQueryParameters || [] === $currentPageQueryParameters) { | ||
| if ($menuItemDto->getLinkUrl() === $currentRequestUri) { | ||
| $menuItemDto->setSelected(true); | ||
| } | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| // if the menu only contains links to the INDEX action of the CRUD controller, | ||
| // match that menu item for all actions (EDIT, NEW, etc.) of the same controller; | ||
| // this is not strictly correct, but backend users expect this behavior because it | ||
| // makes the sidebar menu more predictable and easier to use | ||
| $menuItemController = $menuItemQueryParameters[EA::CRUD_CONTROLLER_FQCN] ?? null; | ||
| $currentPageController = $currentPageQueryParameters[EA::CRUD_CONTROLLER_FQCN] ?? null; | ||
| $actionsLinkedInTheMenuForThisEntity = $controllersAndActionsLinkedInTheMenu[$currentPageController] ?? []; | ||
| $menuOnlyLinksToIndexActionOfThisEntity = $actionsLinkedInTheMenuForThisEntity === [Crud::PAGE_INDEX]; | ||
| if ($menuItemController === $currentPageController && $menuOnlyLinksToIndexActionOfThisEntity) { | ||
| $menuItemDto->setSelected(true); | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| // if the menu contains links to more than one action of the CRUD controller | ||
| // (e.g. INDEX and NEW), and the action of the current page is not included | ||
| // in that list, match the menu item with the INDEX action; this is again not | ||
| // strictly correct, but it's the expected behavior by backend users | ||
| $menuItemAction = $menuItemQueryParameters[EA::CRUD_ACTION] ?? null; | ||
| $currentPageAction = $currentPageQueryParameters[EA::CRUD_ACTION] ?? null; | ||
| $isCurrentPageActionLinkedInTheMenu = \in_array($currentPageAction, $actionsLinkedInTheMenuForThisEntity, true); | ||
| if ($menuItemController === $currentPageController && Crud::PAGE_INDEX === $menuItemAction && !$isCurrentPageActionLinkedInTheMenu) { | ||
| $menuItemDto->setSelected(true); | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| $currentPageQueryParameters = $adminContext->getRequest()->query->all(); | ||
| $menuItemQueryString = null === $menuItemDto->getLinkUrl() ? null : parse_url($menuItemDto->getLinkUrl(), \PHP_URL_QUERY); | ||
| // otherwise, match the query parameters of each menu item with the query | ||
| // parameters of the current request; before making the match, we remove | ||
| // some irrelevant query parameters such as filters, sorting, pagination, etc. | ||
| $menuItemQueryParametersToCompare = $this->filterIrrelevantQueryParameters($menuItemQueryParameters); | ||
| $currentPageQueryParametersToCompare = $this->filterIrrelevantQueryParameters($currentPageQueryParameters); | ||
|
|
||
| $menuItemQueryParameters = []; | ||
| if (\is_string($menuItemQueryString)) { | ||
| parse_str($menuItemQueryString, $menuItemQueryParameters); | ||
| // needed so you can pass route parameters in any order | ||
| $this->recursiveKsort($menuItemQueryParametersToCompare); | ||
| $this->recursiveKsort($currentPageQueryParametersToCompare); | ||
|
|
||
| if ($menuItemQueryParametersToCompare === $currentPageQueryParametersToCompare) { | ||
| $menuItemDto->setSelected(true); | ||
|
|
||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ([] === $menuItemQueryParameters || [] === $currentPageQueryParameters) { | ||
| return $menuItemDto->getLinkUrl() === $adminContext->getRequest()->getUri(); | ||
| return $menuItems; | ||
| } | ||
|
|
||
| /** | ||
| * Given the full list of menu items, this method finds which item should be | ||
| * marked as expanded because any of its items is currently selected and | ||
| * updates it. | ||
| * | ||
| * @param MenuItemDto[] $menuItems | ||
| * | ||
| * @return MenuItemDto[] | ||
| */ | ||
| private function doMarkExpandedMenuItem(array $menuItems): array | ||
| { | ||
| foreach ($menuItems as $menuItemDto) { | ||
| if ([] === $menuSubitems = $menuItemDto->getSubItems()) { | ||
| continue; | ||
| } | ||
|
|
||
| foreach ($menuSubitems as $submenuItem) { | ||
| if ($submenuItem->isSelected()) { | ||
| $menuItemDto->setExpanded(true); | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $menuItemLinksToIndexCrudAction = Crud::PAGE_INDEX === ($menuItemQueryParameters[EA::CRUD_ACTION] ?? false); | ||
| $currentPageLinksToIndexCrudAction = Crud::PAGE_INDEX === ($currentPageQueryParameters[EA::CRUD_ACTION] ?? false); | ||
| $menuItemQueryParameters = $this->filterIrrelevantQueryParameters($menuItemQueryParameters, $menuItemLinksToIndexCrudAction); | ||
| $currentPageQueryParameters = $this->filterIrrelevantQueryParameters($currentPageQueryParameters, $currentPageLinksToIndexCrudAction); | ||
| return $menuItems; | ||
| } | ||
|
|
||
| /** | ||
| * It scans the full list of menu items to find which controllers and actions | ||
| * are linked from the menu. The output is something like: | ||
| * [ | ||
| * 'App\Controller\Admin\BlogPostCrudController' => ['index', 'new'], | ||
| * 'App\Controller\Admin\BlogCategoryCrudController' => ['index'], | ||
| * // ... | ||
| * 'App\Controller\Admin\UserCrudController' => ['index', 'new'], | ||
| * ]. | ||
| * | ||
| * @return array<string, array<string>> | ||
| */ | ||
| private function getControllersAndActionsLinkedInTheMenu(array $menuItems): array | ||
| { | ||
| $controllersAndActionsLinkedInTheMenu = []; | ||
| foreach ($menuItems as $menuItemDto) { | ||
| if ($menuItemDto->isMenuSection()) { | ||
| continue; | ||
| } | ||
|
|
||
| if ([] !== $subItems = $menuItemDto->getSubItems()) { | ||
| $controllersAndActionsLinkedInTheMenu = array_merge_recursive($controllersAndActionsLinkedInTheMenu, $this->getControllersAndActionsLinkedInTheMenu($subItems)); | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| $menuItemQueryString = null === $menuItemDto->getLinkUrl() ? null : parse_url($menuItemDto->getLinkUrl(), \PHP_URL_QUERY); | ||
| if (null === $menuItemQueryString) { | ||
| continue; | ||
| } | ||
|
|
||
| $menuItemQueryParameters = []; | ||
| if (\is_string($menuItemQueryString)) { | ||
| parse_str($menuItemQueryString, $menuItemQueryParameters); | ||
| } | ||
|
|
||
| $controllerFqcn = $menuItemQueryParameters[EA::CRUD_CONTROLLER_FQCN] ?? null; | ||
| $crudAction = $menuItemQueryParameters[EA::CRUD_ACTION] ?? null; | ||
| if (null === $controllerFqcn || null === $crudAction) { | ||
| continue; | ||
| } | ||
|
|
||
| // needed so you can pass route parameters in any order | ||
| sort($menuItemQueryParameters); | ||
| sort($currentPageQueryParameters); | ||
| if (isset($controllersAndActionsLinkedInTheMenu[$controllerFqcn])) { | ||
| $controllersAndActionsLinkedInTheMenu[$controllerFqcn][] = $crudAction; | ||
| } else { | ||
| $controllersAndActionsLinkedInTheMenu[$controllerFqcn] = [$crudAction]; | ||
| } | ||
| } | ||
|
|
||
| return $menuItemQueryParameters === $currentPageQueryParameters; | ||
| return $controllersAndActionsLinkedInTheMenu; | ||
| } | ||
|
|
||
| public function isExpanded(MenuItemDto $menuItemDto): bool | ||
| /* | ||
| * Sorts an array recursively by its keys. This is needed because some values | ||
| * of the array with the query string parameters can be arrays too, and we must | ||
| * sort those before the comparison. | ||
| */ | ||
| private function recursiveKsort(&$array): void | ||
| { | ||
| if ([] === $menuSubitems = $menuItemDto->getSubItems()) { | ||
| return false; | ||
| if (!\is_array($array)) { | ||
| return; | ||
| } | ||
|
|
||
| foreach ($menuSubitems as $submenuItem) { | ||
| if ($submenuItem->isSelected()) { | ||
| return true; | ||
| ksort($array); | ||
|
|
||
| foreach ($array as &$value) { | ||
| if (\is_array($value)) { | ||
| $this->recursiveKsort($value); | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Removes from the given list of query parameters all the parameters that | ||
| * should be ignored when deciding if some menu item matches the current page | ||
| * (such as the applied filters or sorting, the listing page number, etc.). | ||
| */ | ||
| private function filterIrrelevantQueryParameters(array $queryStringParameters, bool $menuItemLinksToIndexCrudAction): array | ||
| private function filterIrrelevantQueryParameters(array $queryStringParameters): array | ||
| { | ||
| $paramsToRemove = [EA::REFERRER, EA::PAGE, EA::FILTERS, EA::SORT]; | ||
|
|
||
| // if the menu item being inspected links to the 'index' action of some entity, | ||
| // remove both the CRUD action and the entity ID from the list of parameters; | ||
| // this way, an 'index' menu item is highlighted for all actions of the same entity; | ||
| // however, if the menu item points to an action different from 'index' (e.g. 'detail', | ||
| // 'new', or 'edit'), only highlight it when the current page points to that same action | ||
| if ($menuItemLinksToIndexCrudAction) { | ||
| $paramsToRemove[] = EA::CRUD_ACTION; | ||
| $paramsToRemove[] = EA::ENTITY_ID; | ||
| } | ||
|
|
||
| return array_filter($queryStringParameters, static fn ($k) => !\in_array($k, $paramsToRemove, true), \ARRAY_FILTER_USE_KEY); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.