bug #6507 Improve the detection of pretty URLs usage (javiereguiluz)

This PR was squashed before being merged into the 4.x branch.

Discussion
----------

Improve the detection of pretty URLs usage

Fixes #6499.

-----

The bug: if you enable pretty URLs but visit a page that doesn't use them (e.g. a Symfony route embedded in a EasyAdmin dashboard), then all URLs of the dashboard (menus, actions, etc.) use ugly URLs.

Why: because we detected if pretty URLs should be used based on the current request. This is wrong.

How to solve this:

1) We could create a config option `easyadmin.use_pretty_urls: true` ... but that would require creating a config file (that soon will be useless because EasyAdmin 5 will only use pretty URLs). It goes a bit against DX, so I don't like that solution.

2) A good technical solution would be to detect if our custom route loader is enabled and it generated the admin routes. Ideally in a Compiler Pass. Sadly, this doesn't work. I asked in Symfony Slack and smart folks like `@stof` confirmed that this can't work.

3) So, I ended up with the following solution:

3.1) The custom route loader now dumps all the generated routes using Symfony's cache component
3.2) We detect if pretty URLs are used by checking if that cached item exists and it's not empty
3.3) Indirectly, this improves a lot the performance of finding the route name for a given tuple of Dashboard + CRUD controller + Action. I was going to do this change in the future, but doing it now will help us solve this issue.

-----

I tested in my apps and everything worked as expected, so I'll try to merge this very soon. Thanks.

Commits
-------

16c0f13 Improve the detection of pretty URLs usage

Author: javiereguiluz

src/Context/AdminContext.php

@@ -43,8 +43,9 @@ final class AdminContext
     private TemplateRegistry $templateRegistry;
     private ?MainMenuDto $mainMenuDto = null;
     private ?UserMenuDto $userMenuDto = null;
+    private bool $usePrettyUrls;
 
-    public function __construct(Request $request, ?UserInterface $user, I18nDto $i18nDto, CrudControllerRegistry $crudControllers, DashboardDto $dashboardDto, DashboardControllerInterface $dashboardController, AssetsDto $assetDto, ?CrudDto $crudDto, ?EntityDto $entityDto, ?SearchDto $searchDto, MenuFactoryInterface $menuFactory, TemplateRegistry $templateRegistry)
+    public function __construct(Request $request, ?UserInterface $user, I18nDto $i18nDto, CrudControllerRegistry $crudControllers, DashboardDto $dashboardDto, DashboardControllerInterface $dashboardController, AssetsDto $assetDto, ?CrudDto $crudDto, ?EntityDto $entityDto, ?SearchDto $searchDto, MenuFactoryInterface $menuFactory, TemplateRegistry $templateRegistry, bool $usePrettyUrls = false)
     {
         $this->request = $request;
         $this->user = $user;
@@ -58,6 +59,7 @@ public function __construct(Request $request, ?UserInterface $user, I18nDto $i18
         $this->searchDto = $searchDto;
         $this->menuFactory = $menuFactory;
         $this->templateRegistry = $templateRegistry;
+        $this->usePrettyUrls = $usePrettyUrls;
     }
 
     public function getRequest(): Request
@@ -109,7 +111,7 @@ public function getAbsoluteUrls(): bool
 
     public function usePrettyUrls(): bool
     {
-        return true === (bool) $this->request->attributes->get(EA::ROUTE_CREATED_BY_EASYADMIN);
+        return $this->usePrettyUrls;
     }
 
     public function getDashboardTitle(): string

src/Factory/AdminContextFactory.php

@@ -20,6 +20,7 @@
 use EasyCorp\Bundle\EasyAdminBundle\Dto\SearchDto;
 use EasyCorp\Bundle\EasyAdminBundle\Registry\CrudControllerRegistry;
 use EasyCorp\Bundle\EasyAdminBundle\Registry\TemplateRegistry;
+use EasyCorp\Bundle\EasyAdminBundle\Router\AdminRouteGenerator;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Symfony\Component\Security\Core\User\UserInterface;
@@ -37,14 +38,16 @@ final class AdminContextFactory
     private MenuFactoryInterface $menuFactory;
     private CrudControllerRegistry $crudControllers;
     private EntityFactory $entityFactory;
+    private AdminRouteGenerator $adminRouteGenerator;
 
-    public function __construct(string $cacheDir, ?TokenStorageInterface $tokenStorage, MenuFactoryInterface $menuFactory, CrudControllerRegistry $crudControllers, EntityFactory $entityFactory)
+    public function __construct(string $cacheDir, ?TokenStorageInterface $tokenStorage, MenuFactoryInterface $menuFactory, CrudControllerRegistry $crudControllers, EntityFactory $entityFactory, AdminRouteGenerator $adminRouteGenerator)
     {
         $this->cacheDir = $cacheDir;
         $this->tokenStorage = $tokenStorage;
         $this->menuFactory = $menuFactory;
         $this->crudControllers = $crudControllers;
         $this->entityFactory = $entityFactory;
+        $this->adminRouteGenerator = $adminRouteGenerator;
     }
 
     public function create(Request $request, DashboardControllerInterface $dashboardController, ?CrudControllerInterface $crudController, ?string $actionName = null): AdminContext
@@ -65,7 +68,9 @@ public function create(Request $request, DashboardControllerInterface $dashboard
         $templateRegistry = $this->getTemplateRegistry($dashboardController, $crudDto);
         $user = $this->getUser($this->tokenStorage);
 
-        return new AdminContext($request, $user, $i18nDto, $this->crudControllers, $dashboardDto, $dashboardController, $assetDto, $crudDto, $entityDto, $searchDto, $this->menuFactory, $templateRegistry);
+        $usePrettyUrls = $this->adminRouteGenerator->usesPrettyUrls();
+
+        return new AdminContext($request, $user, $i18nDto, $this->crudControllers, $dashboardDto, $dashboardController, $assetDto, $crudDto, $entityDto, $searchDto, $this->menuFactory, $templateRegistry, $usePrettyUrls);
     }
 
     private function getDashboardDto(Request $request, DashboardControllerInterface $dashboardControllerInstance): DashboardDto

src/Resources/config/services.php

@@ -185,6 +185,7 @@
             ->arg(2, new Reference(MenuFactory::class))
             ->arg(3, new Reference(CrudControllerRegistry::class))
             ->arg(4, new Reference(EntityFactory::class))
+            ->arg(5, service(AdminRouteGenerator::class))
 
         ->set(AdminUrlGenerator::class)
             // I don't know if we truly need the share() method to get a new instance of the
@@ -200,9 +201,14 @@
             ->args([[AdminUrlGenerator::class => service(AdminUrlGenerator::class)]])
             ->tag('container.service_locator')
 
+        ->set('cache.easyadmin')
+            ->parent('cache.system')
+            ->tag('cache.pool')
+
         ->set(AdminRouteGenerator::class)
             ->arg(0, tagged_iterator(EasyAdminExtension::TAG_DASHBOARD_CONTROLLER))
             ->arg(1, tagged_iterator(EasyAdminExtension::TAG_CRUD_CONTROLLER))
+            ->arg(2, service('cache.easyadmin'))
 
         ->set(AdminRouteLoader::class)
             ->arg(0, service(AdminRouteGenerator::class))

src/Router/AdminRouteGenerator.php

@@ -7,6 +7,7 @@
 use EasyCorp\Bundle\EasyAdminBundle\Attribute\AdminDashboard;
 use EasyCorp\Bundle\EasyAdminBundle\Config\Option\EA;
 use EasyCorp\Bundle\EasyAdminBundle\Contracts\Router\AdminRouteGeneratorInterface;
+use Psr\Cache\CacheItemPoolInterface;
 use Symfony\Component\Routing\Route;
 use Symfony\Component\Routing\RouteCollection;
 
@@ -22,6 +23,7 @@
  */
 final class AdminRouteGenerator implements AdminRouteGeneratorInterface
 {
+    public const ADMIN_ROUTES_CACHE_KEY = 'easyadmin.generated_routes';
     private const DEFAULT_ROUTES_CONFIG = [
         'index' => [
             'routePath' => '/',
@@ -63,13 +65,59 @@ final class AdminRouteGenerator implements AdminRouteGeneratorInterface
     public function __construct(
         private iterable $dashboardControllers,
         private iterable $crudControllers,
+        private CacheItemPoolInterface $cache,
     ) {
     }
 
     public function generateAll(): RouteCollection
     {
         $collection = new RouteCollection();
+        $adminRoutes = $this->generateAdminRoutes();
+
+        foreach ($adminRoutes as $routeName => $route) {
+            $collection->add($routeName, $route);
+        }
+
+        // save the generated routes in the cache; this will allow to detect
+        // if pretty URLs are being used in the application and also improves
+        // performance when finding a route name using the {dashboard, CRUD controller, action} tuple
+        $adminRoutesCache = [];
+        foreach ($adminRoutes as $routeName => $route) {
+            $adminRoutesCache[$route->getOption(EA::DASHBOARD_CONTROLLER_FQCN)][$route->getOption(EA::CRUD_CONTROLLER_FQCN)][$route->getOption(EA::CRUD_ACTION)] = $routeName;
+        }
+        $cachedAdminRoutes = $this->cache->getItem(self::ADMIN_ROUTES_CACHE_KEY);
+        $cachedAdminRoutes->set($adminRoutesCache);
+        $this->cache->save($cachedAdminRoutes);
+
+        return $collection;
+    }
+
+    // Temporary utility method to be removed in EasyAdmin 5, when the pretty URLs will be mandatory
+    // TODO: remove this method in EasyAdmin 5.x
+    public function usesPrettyUrls(): bool
+    {
+        $cachedAdminRoutes = $this->cache->getItem(self::ADMIN_ROUTES_CACHE_KEY)->get();
+
+        return null !== $cachedAdminRoutes && [] !== $cachedAdminRoutes;
+    }
+
+    public function findRouteName(string $dashboardFqcn, string $crudControllerFqcn, string $actionName): ?string
+    {
+        $adminRoutes = $this->cache->getItem(self::ADMIN_ROUTES_CACHE_KEY)->get();
+
+        return $adminRoutes[$dashboardFqcn][$crudControllerFqcn][$actionName] ?? null;
+    }
+
+    /**
+     * @return array<string, Route>
+     */
+    private function generateAdminRoutes(): array
+    {
+        /** @var array<string, Route> $adminRoutes Stores the collection of admin routes created for the app */
+        $adminRoutes = [];
+        /** @var array<string> $addedRouteNames Temporary cache that stores the route names to ensure that we don't add duplicated admin routes */
         $addedRouteNames = [];
+
         foreach ($this->dashboardControllers as $dashboardController) {
             $dashboardFqcn = $dashboardController::class;
             [$allowedCrudControllers, $deniedCrudControllers] = $this->getAllowedAndDeniedControllers($dashboardFqcn);
@@ -104,8 +152,12 @@ public function generateAll(): RouteCollection
 
                 foreach (array_keys($actionsRouteConfig) as $actionName) {
                     $actionRouteConfig = $actionsRouteConfig[$actionName];
-                    $crudActionPath = sprintf('%s/%s/%s', $dashboardRouteConfig['routePath'], $crudControllerRouteConfig['routePath'], ltrim($actionRouteConfig['routePath'], '/'));
-                    $crudActionRouteName = sprintf('%s_%s_%s', $dashboardRouteConfig['routeName'], $crudControllerRouteConfig['routeName'], $actionRouteConfig['routeName']);
+                    $adminRoutePath = sprintf('%s/%s/%s', $dashboardRouteConfig['routePath'], $crudControllerRouteConfig['routePath'], ltrim($actionRouteConfig['routePath'], '/'));
+                    $adminRouteName = sprintf('%s_%s_%s', $dashboardRouteConfig['routeName'], $crudControllerRouteConfig['routeName'], $actionRouteConfig['routeName']);
+
+                    if (\in_array($adminRouteName, $addedRouteNames, true)) {
+                        throw new \RuntimeException(sprintf('When using pretty URLs, all CRUD controllers must have unique PHP class names to generate unique route names. However, your application has at least two controllers with the FQCN "%s", generating the route "%s". Even if both CRUD controllers are in different namespaces, they cannot have the same class name. Rename one of these controllers to resolve the issue.', $crudControllerFqcn, $adminRouteName));
+                    }
 
                     $defaults = [
                         '_controller' => $crudControllerFqcn.'::'.$actionName,
@@ -117,34 +169,14 @@ public function generateAll(): RouteCollection
                         EA::CRUD_ACTION => $actionName,
                     ];
 
-                    $route = new Route($crudActionPath, defaults: $defaults, options: $options, methods: $actionRouteConfig['methods']);
-
-                    if (\in_array($crudActionRouteName, $addedRouteNames, true)) {
-                        throw new \RuntimeException(sprintf('When using pretty URLs, all CRUD controllers must have unique PHP class names to generate unique route names. However, your application has at least two controllers with the FQCN "%s", generating the route "%s". Even if both CRUD controllers are in different namespaces, they cannot have the same class name. Rename one of these controllers to resolve the issue.', $crudControllerFqcn, $crudActionRouteName));
-                    }
-
-                    $collection->add($crudActionRouteName, $route);
-                    $addedRouteNames[] = $crudActionRouteName;
+                    $adminRoute = new Route($adminRoutePath, defaults: $defaults, options: $options, methods: $actionRouteConfig['methods']);
+                    $adminRoutes[$adminRouteName] = $adminRoute;
+                    $addedRouteNames[] = $adminRouteName;
                 }
             }
         }
 
-        return $collection;
-    }
-
-    public function findRouteName(string $dashboardFqcn, string $crudControllerFqcn, string $actionName): ?string
-    {
-        $defaultRoutesConfig = $this->getDefaultRoutesConfig($dashboardFqcn);
-        $actionsRouteConfig = array_replace_recursive($defaultRoutesConfig, $this->getCustomActionsConfig($crudControllerFqcn));
-        if (!isset($actionsRouteConfig[$actionName])) {
-            return null;
-        }
-
-        $dashboardRouteConfig = $this->getDashboardsRouteConfig()[$dashboardFqcn];
-        $crudControllerRouteConfig = $this->getCrudControllerRouteConfig($crudControllerFqcn);
-        $actionRouteConfig = $actionsRouteConfig[$actionName];
-
-        return sprintf('%s_%s_%s', $dashboardRouteConfig['routeName'], $crudControllerRouteConfig['routeName'], $actionRouteConfig['routeName']);
+        return $adminRoutes;
     }
 
     /**