Refactor MenuComposer to improve type safety and simplify local menu builder integration

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);
     }

src/Bundle/ChillMainBundle/Resources/views/Menu/section.html.twig

@@ -1,34 +1,16 @@
-{#
- * Copyright (C) 2014-2015, Champs Libres Cooperative SCRLFS, 
- <info@champs-libres.coop> / <http://www.champs-libres.coop>
- *
- * 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 <http://www.gnu.org/licenses/>.
-#}
-
 <li class="nav-item dropdown btn btn-primary nav-section">
-    <a id="menu-section" 
+    <a id="menu-section"
-        class="nav-link dropdown-toggle" 
+        class="nav-link dropdown-toggle"
-        type="button" 
+        type="button"
-        data-bs-toggle="dropdown" 
+        data-bs-toggle="dropdown"
-        aria-haspopup="true" 
+        aria-haspopup="true"
         aria-expanded="false">
-        
+
         {{ 'Sections'|trans }}
     </a>
     <div class="dropdown-menu dropdown-menu-end dropdown-menu-dark" aria-labelledby="menu-section">
         {% for menu in menus %}
-            <a class="dropdown-item list-group-item bg-dark text-white" 
+            <a class="dropdown-item list-group-item bg-dark text-white"
                 href="{{ menu.uri }}">
                 {{ menu.label }}
                 {% apply spaceless %}

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<LocalMenuBuilderInterface>
+         */
+        private iterable $localMenuBuilders,
+    ) {}
 
-    private RouteCollection $routeCollection;
+    public function getMenuFor($menuId, array $parameters = []): ItemInterface
-
-    public function __construct(private readonly RouterInterface $router, private readonly FactoryInterface $menuFactory, private readonly TranslatorInterface $translator) {}
-
-    public function addLocalMenuBuilder(LocalMenuBuilderInterface $menuBuilder, $menuId)
     {
-        $this->localMenuBuilders[$menuId][] = $menuBuilder;
+        $routes = $this->getRoutesForInternal($menuId, $parameters);
-    }
+        /** @var MenuItem $menu */
-
-    public function getMenuFor($menuId, array $parameters = [])
-    {
-        $routes = $this->getRoutesFor($menuId, $parameters);
         $menu = $this->menuFactory->createItem($menuId);
 
         // build menu from routes
@@ -55,10 +53,9 @@ class MenuComposer
                 ]);
         }
 
-        if ($this->hasLocalMenuBuilder($menuId)) {
+        foreach ($this->localMenuBuilders as $builder) {
-            foreach ($this->localMenuBuilders[$menuId] as $builder) {
+            if (in_array($menuId, $builder::getMenuIds(), true)) {
-                /* @var $builder LocalMenuBuilderInterface */
+                $builder->buildMenu($menuId, $menu, $parameters);
-                $builder->buildMenu($menuId, $menu, $parameters['args']);
             }
         }
 
@@ -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
+     * @deprecated
-     * @param array  $parameters see https://redmine.champs-libres.coop/issues/179
      *
-     * @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;
+            }
+        }
 
-    /**
+        return false;
-     * 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;
     }
 
     private function reorderMenu(ItemInterface $menu)

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 array{layout?: string, activeRouteKey?: string|null, args?: array<array-key, mixed>} $params
-     * @param mixed[] $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['menus'] = $resolvedParams['routes'] = $this->menuComposer->getMenuFor($menuId, $resolvedParams['args']);
-            $resolvedParams['routes'] = $this->menuComposer->getRoutesFor($menuId, $resolvedParams);
-
-            return $env->render($layout, $resolvedParams);
-        }
-        $resolvedParams['menus'] = $this->menuComposer->getMenuFor($menuId, $resolvedParams);
 
         return $env->render($layout, $resolvedParams);
     }

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
 {
-    /**
+    use ProphecyTrait;
-     * @var \Symfony\Bundle\FrameworkBundle\Routing\DelegatingLoader;
-     */
-    private $loader;
 
-    /**
+    private function buildMenuComposerWithDefaultBuilder(): array
-     * @var \Chill\MainBundle\DependencyInjection\Services\MenuComposer;
-     */
-    private $menuComposer;
-
-    protected function setUp(): void
     {
-        self::bootKernel(['environment' => 'test']);
+        // Router: returns an empty RouteCollection so getRoutesFor() yields []
-        $this->menuComposer = self::getContainer()
+        $routerProphecy = $this->prophesize(RouterInterface::class);
-            ->get('chill.main.menu_composer');
+        $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];
     }
 
-    /**
+    public function testGetMenuForReturnsItemsFromLocalBuildersOnly(): void
-     * @covers \Chill\MainBundle\Routing\MenuComposer
-     */
-    public function testMenuComposer()
     {
-        $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'));
     }
 }

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'
+            $localMenuBuilders: !tagged_iterator 'chill.menu_builder'
-            - '@Knp\Menu\FactoryInterface'
+
-            - '@Symfony\Contracts\Translation\TranslatorInterface'
     Chill\MainBundle\Routing\MenuComposer: '@chill.main.menu_composer'
 
     chill.main.routes_loader: