Merge branch 'ticket/improve-local-menu-builder' into 'ticket-app-master'

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

See merge request Chill-Projet/chill-bundles!890

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,21 +1,3 @@
-{#
- * 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"
         class="nav-link dropdown-toggle"

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 __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)

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<array-key, 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['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);
     }

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

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: