From 3928b2cc7ac43174738bb4d93404452d015a535a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Mon, 29 Sep 2025 15:03:05 +0000 Subject: [PATCH] Refactor `MenuComposer` to improve type safety and simplify local menu builder integration --- .../ChillMainBundle/ChillMainBundle.php | 2 - .../Resources/views/Menu/section.html.twig | 32 ++--- .../ChillMainBundle/Routing/MenuComposer.php | 70 ++++++----- .../ChillMainBundle/Routing/MenuTwig.php | 14 +-- .../Tests/Services/MenuComposerTest.php | 109 ++++++++++++++---- .../config/services/routing.yaml | 5 +- 6 files changed, 132 insertions(+), 100 deletions(-) diff --git a/src/Bundle/ChillMainBundle/ChillMainBundle.php b/src/Bundle/ChillMainBundle/ChillMainBundle.php index 4a059662c..8d2969ee3 100644 --- a/src/Bundle/ChillMainBundle/ChillMainBundle.php +++ b/src/Bundle/ChillMainBundle/ChillMainBundle.php @@ -14,7 +14,6 @@ namespace Chill\MainBundle; use Chill\MainBundle\Cron\CronJobInterface; use Chill\MainBundle\CRUD\CompilerPass\CRUDControllerCompilerPass; use Chill\MainBundle\DependencyInjection\CompilerPass\ACLFlagsCompilerPass; -use Chill\MainBundle\DependencyInjection\CompilerPass\MenuCompilerPass; use Chill\MainBundle\DependencyInjection\CompilerPass\NotificationCounterCompilerPass; use Chill\MainBundle\DependencyInjection\CompilerPass\SearchableServicesCompilerPass; use Chill\MainBundle\DependencyInjection\CompilerPass\TimelineCompilerClass; @@ -70,7 +69,6 @@ class ChillMainBundle extends Bundle $container->addCompilerPass(new TimelineCompilerClass(), \Symfony\Component\DependencyInjection\Compiler\PassConfig::TYPE_BEFORE_OPTIMIZATION, 0); $container->addCompilerPass(new WidgetsCompilerPass(), \Symfony\Component\DependencyInjection\Compiler\PassConfig::TYPE_BEFORE_OPTIMIZATION, 0); $container->addCompilerPass(new NotificationCounterCompilerPass(), \Symfony\Component\DependencyInjection\Compiler\PassConfig::TYPE_BEFORE_OPTIMIZATION, 0); - $container->addCompilerPass(new MenuCompilerPass(), \Symfony\Component\DependencyInjection\Compiler\PassConfig::TYPE_BEFORE_OPTIMIZATION, 0); $container->addCompilerPass(new ACLFlagsCompilerPass(), \Symfony\Component\DependencyInjection\Compiler\PassConfig::TYPE_BEFORE_OPTIMIZATION, 0); $container->addCompilerPass(new CRUDControllerCompilerPass(), \Symfony\Component\DependencyInjection\Compiler\PassConfig::TYPE_BEFORE_OPTIMIZATION, 0); } diff --git a/src/Bundle/ChillMainBundle/Resources/views/Menu/section.html.twig b/src/Bundle/ChillMainBundle/Resources/views/Menu/section.html.twig index 67683bf50..56bb9a2a9 100644 --- a/src/Bundle/ChillMainBundle/Resources/views/Menu/section.html.twig +++ b/src/Bundle/ChillMainBundle/Resources/views/Menu/section.html.twig @@ -1,34 +1,16 @@ -{# - * Copyright (C) 2014-2015, Champs Libres Cooperative SCRLFS, - / - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as - * published by the Free Software Foundation, either version 3 of the - * License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . -#} -
  • -
    {% for menu in menus %} - {{ menu.label }} {% apply spaceless %} diff --git a/src/Bundle/ChillMainBundle/Routing/MenuComposer.php b/src/Bundle/ChillMainBundle/Routing/MenuComposer.php index e16b10e8f..101a97f6e 100644 --- a/src/Bundle/ChillMainBundle/Routing/MenuComposer.php +++ b/src/Bundle/ChillMainBundle/Routing/MenuComposer.php @@ -13,32 +13,30 @@ namespace Chill\MainBundle\Routing; use Knp\Menu\FactoryInterface; use Knp\Menu\ItemInterface; -use Symfony\Component\Routing\RouteCollection; +use Knp\Menu\MenuItem; use Symfony\Component\Routing\RouterInterface; use Symfony\Contracts\Translation\TranslatorInterface; /** * This class permit to build menu from the routing information * stored in each bundle. - * - * how to must come here FIXME */ -class MenuComposer +final readonly class MenuComposer { - private array $localMenuBuilders = []; + public function __construct( + private RouterInterface $router, + private FactoryInterface $menuFactory, + private TranslatorInterface $translator, + /** + * @var iterable + */ + private iterable $localMenuBuilders, + ) {} - private RouteCollection $routeCollection; - - public function __construct(private readonly RouterInterface $router, private readonly FactoryInterface $menuFactory, private readonly TranslatorInterface $translator) {} - - public function addLocalMenuBuilder(LocalMenuBuilderInterface $menuBuilder, $menuId) + public function getMenuFor($menuId, array $parameters = []): ItemInterface { - $this->localMenuBuilders[$menuId][] = $menuBuilder; - } - - public function getMenuFor($menuId, array $parameters = []) - { - $routes = $this->getRoutesFor($menuId, $parameters); + $routes = $this->getRoutesForInternal($menuId, $parameters); + /** @var MenuItem $menu */ $menu = $this->menuFactory->createItem($menuId); // build menu from routes @@ -55,10 +53,9 @@ class MenuComposer ]); } - if ($this->hasLocalMenuBuilder($menuId)) { - foreach ($this->localMenuBuilders[$menuId] as $builder) { - /* @var $builder LocalMenuBuilderInterface */ - $builder->buildMenu($menuId, $menu, $parameters['args']); + foreach ($this->localMenuBuilders as $builder) { + if (in_array($menuId, $builder::getMenuIds(), true)) { + $builder->buildMenu($menuId, $menu, $parameters); } } @@ -71,12 +68,16 @@ class MenuComposer * Return an array of routes added to $menuId, * The array is aimed to build route with MenuTwig. * - * @param string $menuId - * @param array $parameters see https://redmine.champs-libres.coop/issues/179 + * @deprecated * - * @return array + * @param array $parameters see https://redmine.champs-libres.coop/issues/179 */ - public function getRoutesFor($menuId, array $parameters = []) + public function getRoutesFor(string $menuId, array $parameters = []): array + { + return $this->getRoutesForInternal($menuId, $parameters); + } + + private function getRoutesForInternal(string $menuId, array $parameters = []): array { $routes = []; $routeCollection = $this->router->getRouteCollection(); @@ -108,22 +109,17 @@ class MenuComposer * should be used, or `getRouteFor`. The method `getMenuFor` should be used * if the result is true (it **does** exists at least one menu builder. * - * @param string $menuId + * @deprecated */ - public function hasLocalMenuBuilder($menuId): bool + public function hasLocalMenuBuilder(string $menuId): bool { - return \array_key_exists($menuId, $this->localMenuBuilders); - } + foreach ($this->localMenuBuilders as $localMenuBuilder) { + if (in_array($menuId, $localMenuBuilder::getMenuIds(), true)) { + return true; + } + } - /** - * Set the route Collection - * This function is needed for testing purpose: routeCollection is not - * available as a service (RouterInterface is provided as a service and - * added to this class as paramater in __construct). - */ - public function setRouteCollection(RouteCollection $routeCollection) - { - $this->routeCollection = $routeCollection; + return false; } private function reorderMenu(ItemInterface $menu) diff --git a/src/Bundle/ChillMainBundle/Routing/MenuTwig.php b/src/Bundle/ChillMainBundle/Routing/MenuTwig.php index 3b6ce7e02..409dcca11 100644 --- a/src/Bundle/ChillMainBundle/Routing/MenuTwig.php +++ b/src/Bundle/ChillMainBundle/Routing/MenuTwig.php @@ -18,7 +18,7 @@ use Twig\TwigFunction; /** * Add the filter 'chill_menu'. */ -class MenuTwig extends AbstractExtension +final class MenuTwig extends AbstractExtension { /** * the default parameters for chillMenu. @@ -43,22 +43,16 @@ class MenuTwig extends AbstractExtension * * @deprecated link: see https://redmine.champs-libres.coop/issues/179 for more informations * - * @param string $menuId - * @param mixed[] $params + * @param array{layout?: string, activeRouteKey?: string|null, args?: array} $params */ - public function chillMenu(Environment $env, $menuId, array $params = []) + public function chillMenu(Environment $env, string $menuId, array $params = []): string { $resolvedParams = array_merge($this->defaultParams, $params); $layout = $resolvedParams['layout']; unset($resolvedParams['layout']); - if (false === $this->menuComposer->hasLocalMenuBuilder($menuId)) { - $resolvedParams['routes'] = $this->menuComposer->getRoutesFor($menuId, $resolvedParams); - - return $env->render($layout, $resolvedParams); - } - $resolvedParams['menus'] = $this->menuComposer->getMenuFor($menuId, $resolvedParams); + $resolvedParams['menus'] = $resolvedParams['routes'] = $this->menuComposer->getMenuFor($menuId, $resolvedParams['args']); return $env->render($layout, $resolvedParams); } diff --git a/src/Bundle/ChillMainBundle/Tests/Services/MenuComposerTest.php b/src/Bundle/ChillMainBundle/Tests/Services/MenuComposerTest.php index ae63557eb..7be02c7e4 100644 --- a/src/Bundle/ChillMainBundle/Tests/Services/MenuComposerTest.php +++ b/src/Bundle/ChillMainBundle/Tests/Services/MenuComposerTest.php @@ -11,44 +11,107 @@ declare(strict_types=1); namespace Chill\MainBundle\Tests\Services; -use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase; +use Chill\MainBundle\Routing\LocalMenuBuilderInterface; +use Chill\MainBundle\Routing\MenuComposer; +use Knp\Menu\MenuFactory; +use PHPUnit\Framework\TestCase; +use Prophecy\PhpUnit\ProphecyTrait; +use Symfony\Component\Routing\RouterInterface; use Symfony\Component\Routing\RouteCollection; +use Symfony\Contracts\Translation\TranslatorInterface; /** - * This class provide functional test for MenuComposer. + * Tests for MenuComposer methods. + * + * We only verify that items provided by local menu builders are present + * when getRoutesFor() yields no routes, and that hasLocalMenuBuilder behaves + * as expected with the configured builders. * * @internal * * @coversNothing */ -final class MenuComposerTest extends KernelTestCase +final class MenuComposerTest extends TestCase { - /** - * @var \Symfony\Bundle\FrameworkBundle\Routing\DelegatingLoader; - */ - private $loader; + use ProphecyTrait; - /** - * @var \Chill\MainBundle\DependencyInjection\Services\MenuComposer; - */ - private $menuComposer; - - protected function setUp(): void + private function buildMenuComposerWithDefaultBuilder(): array { - self::bootKernel(['environment' => 'test']); - $this->menuComposer = self::getContainer() - ->get('chill.main.menu_composer'); + // Router: returns an empty RouteCollection so getRoutesFor() yields [] + $routerProphecy = $this->prophesize(RouterInterface::class); + $routerProphecy->getRouteCollection()->willReturn(new RouteCollection()); + $router = $routerProphecy->reveal(); + + // Menu factory from Knp\Menu + $menuFactory = new MenuFactory(); + + // Translator: identity translator + $translator = new class () implements TranslatorInterface { + public function trans(string $id, array $parameters = [], ?string $domain = null, ?string $locale = null): string + { + return $id; + } + + public function getLocale(): string + { + return 'en'; + } + }; + + // Local builder that adds two items to the requested menu + $builder = new class () implements LocalMenuBuilderInterface { + public static function getMenuIds(): array + { + return ['main']; + } + + public function buildMenu($menuId, \Knp\Menu\MenuItem $menu, array $parameters) + { + // Ensure we can use parameters passed to getMenuFor + $suffix = $parameters['suffix'] ?? ''; + $menu->addChild('local_item_one', [ + 'label' => 'Local Item One'.$suffix, + ])->setExtras(['order' => 1]); + $menu->addChild('local_item_two', [ + 'label' => 'Local Item Two'.$suffix, + ])->setExtras(['order' => 2]); + } + }; + + $composer = new MenuComposer( + $router, + $menuFactory, + $translator, + [$builder] + ); + + return [$composer, $builder]; } - /** - * @covers \Chill\MainBundle\Routing\MenuComposer - */ - public function testMenuComposer() + public function testGetMenuForReturnsItemsFromLocalBuildersOnly(): void { - $collection = new RouteCollection(); + [$composer] = $this->buildMenuComposerWithDefaultBuilder(); - $routes = $this->menuComposer->getRoutesFor('dummy0'); + $menu = $composer->getMenuFor('main', []); - $this->assertIsArray($routes); + // No routes were added, only local builder items should be present + $children = $menu->getChildren(); + self::assertCount(2, $children, 'Menu should contain exactly the items provided by local builders'); + + // Assert the two expected items exist with their names + self::assertNotNull($menu->getChild('local_item_one')); + self::assertNotNull($menu->getChild('local_item_two')); + + // And that their labels include the parameter suffix + self::assertSame('Local Item One', $menu->getChild('local_item_one')->getLabel()); + self::assertSame('Local Item Two', $menu->getChild('local_item_two')->getLabel()); + } + + public function testHasLocalMenuBuilder(): void + { + [$composer] = $this->buildMenuComposerWithDefaultBuilder(); + + self::assertTrue($composer->hasLocalMenuBuilder('main')); + self::assertFalse($composer->hasLocalMenuBuilder('secondary')); } } diff --git a/src/Bundle/ChillMainBundle/config/services/routing.yaml b/src/Bundle/ChillMainBundle/config/services/routing.yaml index 7d5a80358..1ade9a6b3 100644 --- a/src/Bundle/ChillMainBundle/config/services/routing.yaml +++ b/src/Bundle/ChillMainBundle/config/services/routing.yaml @@ -6,9 +6,8 @@ services: chill.main.menu_composer: class: Chill\MainBundle\Routing\MenuComposer arguments: - - '@Symfony\Component\Routing\RouterInterface' - - '@Knp\Menu\FactoryInterface' - - '@Symfony\Contracts\Translation\TranslatorInterface' + $localMenuBuilders: !tagged_iterator 'chill.menu_builder' + Chill\MainBundle\Routing\MenuComposer: '@chill.main.menu_composer' chill.main.routes_loader: