Merge pull request #3444 from insiders/domain-router

[MultidomainBundle] Improve domain based locale router performance

UPGRADE-7.2.md

@@ -10,3 +10,10 @@ PagePartBundle
 --------------
 
 - Not passing a HasPagePartsInterface as second parameter in PagePartEvent is deprecated and will be required in 8.0.
+
+
+NodeBundle
+-----------------
+
+- The node-bundle/multidomain-bundle has now some improved logic in the router itself. Using the old router logic is deprecated and the new will be the default in 8.0.
+  To enable the new and improved router, set the `kunstmaan_node.enable_improved_router` config to `true`.

src/Kunstmaan/MultiDomainBundle/DependencyInjection/Configuration.php

@@ -52,6 +52,7 @@ public function getConfigTreeBuilder(): TreeBuilder
                                 ->end()
                             ->end()
                         ->end()
+                    ->end()
                 ->end()
             ->end();
 

src/Kunstmaan/MultiDomainBundle/Router/DomainBasedLocaleRouter.php

@@ -14,18 +14,15 @@
 
 class DomainBasedLocaleRouter extends SlugRouter
 {
-    /** @var RouteCollection */
-    protected $routeCollectionMultiLanguage;
-
     /**
-     * @var array|null
+     * @var RouteCollection
+     *
+     * @deprecated routeCollectionMultiLanguage property is deprecated in 7.2 and will be removed in 8.0. There is no replacement for this property.
      */
-    private $otherSite;
+    protected $routeCollectionMultiLanguage;
 
-    /**
-     * @var array
-     */
-    private $cachedNodeTranslations = [];
+    private ?array $otherSite = null;
+    private array $cachedNodeTranslations = [];
 
     /**
      * Generate an url for a supplied route
@@ -36,7 +33,7 @@ class DomainBasedLocaleRouter extends SlugRouter
      */
     public function generate($name, $parameters = [], $referenceType = UrlGeneratorInterface::ABSOLUTE_PATH): string
     {
-        if ('_slug' === $name && $this->isMultiLanguage() && $this->isMultiDomainHost()) {
+        if ('_slug' === $name && $this->isMultiDomainHost() && $this->isMultiLanguage()) {
             $locale = isset($parameters['_locale']) ? $parameters['_locale'] : $this->getRequestLocale();
 
             $reverseLocaleMap = $this->getReverseLocaleMap();
@@ -47,8 +44,6 @@ public function generate($name, $parameters = [], $referenceType = UrlGeneratorI
 
         if (isset($parameters['otherSite'])) {
             $this->otherSite = $this->domainConfiguration->getFullHostById($parameters['otherSite']);
-        } else {
-            $this->otherSite = null;
         }
 
         $this->urlGenerator = new UrlGenerator(
@@ -80,13 +75,8 @@ public function match($pathinfo): array
         $result = $urlMatcher->match($pathinfo);
         if (!empty($result)) {
             // Remap locale for front-end requests
-            if ($this->isMultiDomainHost()
-                && $this->isMultiLanguage()
-                && !$result['preview']
-            ) {
-                $localeMap = $this->getLocaleMap();
-                $locale = $result['_locale'];
-                $result['_locale'] = $localeMap[$locale];
+            if (!$result['preview'] && $this->isMultiDomainHost() && $this->isMultiLanguage()) {
+                $result['_locale'] = $this->getLocaleMap()[$result['_locale']];
             }
 
             $nodeTranslation = $this->getNodeTranslation($result);
@@ -104,13 +94,7 @@ public function match($pathinfo): array
      */
     protected function getRequestLocale()
     {
-        $request = $this->getMasterRequest();
-        $locale = $this->getDefaultLocale();
-        if (!\is_null($request)) {
-            $locale = $request->getLocale();
-        }
-
-        return $locale;
+        return $this->getMasterRequest()?->getLocale() ?: $this->getDefaultLocale();
     }
 
     /**
@@ -176,15 +160,19 @@ private function getReverseLocaleMap(): array
      */
     public function getRouteCollection(): RouteCollection
     {
-        if (($this->otherSite && $this->isMultiLanguage($this->otherSite['host'])) || (!$this->otherSite && $this->isMultiLanguage())) {
-            if (!$this->routeCollectionMultiLanguage) {
-                $this->routeCollectionMultiLanguage = new RouteCollection();
+        if (!$this->enabledImprovedRouter) {
+            trigger_deprecation('kunstmaan/multidomain-bundle', '7.2', 'Not enabling the improved router is deprecated and the changed and improved router will be the default in 8.0. Set the "kunstmaan_node.enable_improved_router" config to true.');
 
-                $this->addMultiLangPreviewRoute();
-                $this->addMultiLangSlugRoute();
-            }
+            if (($this->otherSite && $this->isMultiLanguage($this->otherSite['host'])) || (!$this->otherSite && $this->isMultiLanguage())) {
+                if (!$this->routeCollectionMultiLanguage) {
+                    $this->routeCollectionMultiLanguage = new RouteCollection();
 
-            return $this->routeCollectionMultiLanguage;
+                    $this->addMultiLangPreviewRoute();
+                    $this->addMultiLangSlugRoute();
+                }
+
+                return $this->routeCollectionMultiLanguage;
+            }
         }
 
         if (!$this->routeCollection) {
@@ -211,6 +199,8 @@ protected function addSlugRoute()
      */
     protected function addMultiLangPreviewRoute()
     {
+        trigger_deprecation('kunstmaan/multidomain-bundle', '7.2', 'This method is deprecated and will be removed in 8.0.');
+
         $routeParameters = $this->getPreviewRouteParameters();
         $this->addMultiLangRoute('_slug_preview', $routeParameters);
     }
@@ -220,6 +210,8 @@ protected function addMultiLangPreviewRoute()
      */
     protected function addMultiLangSlugRoute()
     {
+        trigger_deprecation('kunstmaan/multidomain-bundle', '7.2', 'This method is deprecated and will be removed in 8.0.');
+
         $routeParameters = $this->getSlugRouteParameters();
         $this->addMultiLangRoute('_slug', $routeParameters);
     }
@@ -229,6 +221,8 @@ protected function addMultiLangSlugRoute()
      */
     protected function addMultiLangRoute($name, array $parameters = [])
     {
+        trigger_deprecation('kunstmaan/multidomain-bundle', '7.2', 'This method is deprecated and will be removed in 8.0.');
+
         $this->routeCollectionMultiLanguage->add(
             $name,
             new Route(
@@ -251,26 +245,25 @@ protected function getSlugRouteParameters()
             '_controller' => SlugController::class . '::slugAction',
             'preview' => false,
             'url' => '',
-            '_locale' => $this->getDefaultLocale(),
         ];
         $slugRequirements = [
             'url' => $this->getSlugPattern(),
         ];
 
-        $locales = [];
-
         // If other site provided and multilingual, get the locales from the host config.
+        $locales = [];
         if ($this->otherSite && $this->isMultiLanguage($this->otherSite['host'])) {
             $locales = $this->getHostLocales();
-        } elseif ($this->isMultiLanguage() && !$this->otherSite) {
+        } elseif (!$this->otherSite && $this->isMultiLanguage()) {
             $locales = $this->getFrontendLocales();
         }
 
-        // Make locale availables for the slug routes.
+        // Make locale available for the slug routes.
         if (!empty($locales)) {
             $slugPath = '/{_locale}' . $slugPath;
-            unset($slugDefaults['_locale']);
-            $slugRequirements['_locale'] = $this->getEscapedLocales($this->getHostLocales());
+            $slugRequirements['_locale'] = $this->getEscapedLocales(!$this->otherSite ? $locales : $this->getHostLocales());
+        } else {
+            $slugDefaults['_locale'] = $this->getDefaultLocale();
         }
 
         return [

src/Kunstmaan/MultiDomainBundle/Tests/Router/DomainBasedLocaleRouterTest.php

@@ -97,6 +97,7 @@ public function testAddMultiLangSlugRoute()
         $request = $this->getRequest('http://singlelangdomain.tld/');
 
         $object = new DomainBasedLocaleRouter($domainConfiguration, $this->getRequestStack($request), $this->getEntityManager(new NodeTranslation()), 'admin');
+        $object->enabledImprovedRouter(true);
 
         $mirror = new \ReflectionClass(DomainBasedLocaleRouter::class);
         $property = $mirror->getProperty('otherSite');
@@ -140,6 +141,7 @@ public function testGetRouteCollection()
 
         /** @var Container $container */
         $object = new DomainBasedLocaleRouter($domainConfiguration, $this->getRequestStack($request), $this->getEntityManager(new NodeTranslation()), 'admin');
+        $object->enabledImprovedRouter(true);
         $mirror = new \ReflectionClass(DomainBasedLocaleRouter::class);
         $property = $mirror->getProperty('otherSite');
         $property->setAccessible(true);
@@ -216,6 +218,9 @@ private function getNodeTranslationRepository($nodeTranslation = null)
 
     private function getDomainBasedLocaleRouter($request, $nodeTranslation = null)
     {
-        return new DomainBasedLocaleRouter($this->getDomainConfiguration(), $this->getRequestStack($request), $this->getEntityManager($nodeTranslation), 'admin');
+        $router = new DomainBasedLocaleRouter($this->getDomainConfiguration(), $this->getRequestStack($request), $this->getEntityManager($nodeTranslation), 'admin');
+        $router->enabledImprovedRouter(true);
+
+        return $router;
     }
 }

src/Kunstmaan/NodeBundle/DependencyInjection/Configuration.php

@@ -57,6 +57,7 @@ public function getConfigTreeBuilder(): TreeBuilder
                         ->booleanNode('enabled')->defaultFalse()->end()
                     ->end()
                 ->end()
+                ->booleanNode('enable_improved_router')->defaultFalse()->end()
             ->end();
 
         return $treeBuilder;

src/Kunstmaan/NodeBundle/DependencyInjection/KunstmaanNodeExtension.php

@@ -42,6 +42,13 @@ public function load(array $configs, ContainerBuilder $container): void
 
         $loader->load('services.yml');
         $loader->load('commands.yml');
+
+        $enableImprovedRouter = $config['enable_improved_router'] ?? false;
+        if (!$enableImprovedRouter) {
+            trigger_deprecation('kunstmaan/node-bundle', '7.2', 'Not setting the "kunstmaan_node.enable_improved_router" config to true is deprecated, it will always be true in 8.0.');
+        }
+        $slugRouter = $container->findDefinition('kunstmaan_node.slugrouter');
+        $slugRouter->addMethodCall('enabledImprovedRouter', [$enableImprovedRouter]);
     }
 
     public function prepend(ContainerBuilder $container): void

src/Kunstmaan/NodeBundle/Router/SlugRouter.php

@@ -52,6 +52,13 @@ class SlugRouter implements RouterInterface
     /** @var string */
     protected $slugPattern;
 
+    /**
+     * NEXT_MAJOR: Remove property
+     *
+     * @internal
+     */
+    protected bool $enabledImprovedRouter = false;
+
     public function __construct(
         DomainConfigurationInterface $domainConfiguration,
         RequestStack $requestStack,
@@ -186,16 +193,16 @@ protected function getPreviewRouteParameters()
             '_controller' => SlugController::class . '::slugAction',
             'preview' => true,
             'url' => '',
-            '_locale' => $this->getDefaultLocale(),
         ];
         $previewRequirements = [
             'url' => $this->getSlugPattern(),
         ];
 
         if ($this->isMultiLanguage()) {
             $previewPath = '/{_locale}' . $previewPath;
-            unset($previewDefaults['_locale']);
             $previewRequirements['_locale'] = $this->getEscapedLocales($this->getBackendLocales());
+        } else {
+            $previewDefaults['_locale'] = $this->getDefaultLocale();
         }
 
         return [
@@ -217,16 +224,16 @@ protected function getSlugRouteParameters()
             '_controller' => SlugController::class . '::slugAction',
             'preview' => false,
             'url' => '',
-            '_locale' => $this->getDefaultLocale(),
         ];
         $slugRequirements = [
             'url' => $this->getSlugPattern(),
         ];
 
         if ($this->isMultiLanguage()) {
             $slugPath = '/{_locale}' . $slugPath;
-            unset($slugDefaults['_locale']);
             $slugRequirements['_locale'] = $this->getEscapedLocales($this->getFrontendLocales());
+        } else {
+            $slugDefaults['_locale'] = $this->getDefaultLocale();
         }
 
         return [
@@ -345,4 +352,14 @@ protected function getEscapedLocales($locales)
 
         return implode('|', $escapedLocales);
     }
+
+    /**
+     * NEXT_MAJOR: Remove method
+     *
+     * @interal
+     */
+    public function enabledImprovedRouter(bool $enabled): void
+    {
+        $this->enabledImprovedRouter = $enabled;
+    }
 }

src/Kunstmaan/NodeBundle/Tests/DependencyInjection/ConfigurationTest.php

@@ -30,6 +30,7 @@ public function testConfigGeneratesAsExpected()
                 'threshold' => 35,
             ],
             'enable_permissions' => true,
+            'enable_improved_router' => false,
         ];
 
         $this->assertProcessedConfigurationEquals([$array], $array);

src/Kunstmaan/NodeBundle/Tests/DependencyInjection/KunstmaanNodeExtensionTest.php

@@ -4,10 +4,13 @@
 
 use Kunstmaan\NodeBundle\DependencyInjection\KunstmaanNodeExtension;
 use Matthias\SymfonyDependencyInjectionTest\PhpUnit\AbstractExtensionTestCase;
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
 use Symfony\Component\DependencyInjection\Extension\ExtensionInterface;
 
 class KunstmaanNodeExtensionTest extends AbstractExtensionTestCase
 {
+    use ExpectDeprecationTrait;
+
     /**
      * @return ExtensionInterface[]
      */
@@ -19,7 +22,7 @@ protected function getContainerExtensions(): array
     public function testCorrectParametersHaveBeenSet()
     {
         $this->container->setParameter('twig.form.resources', []);
-        $this->load();
+        $this->load(['enable_improved_router' => true]);
 
         $this->assertContainerBuilderHasParameter('twig.form.resources');
         $this->assertContainerBuilderHasParameter('kunstmaan_node.show_add_homepage', true);
@@ -30,4 +33,14 @@ public function testCorrectParametersHaveBeenSet()
         $this->assertContainerBuilderHasParameter('kunstmaan_node.version_timeout', 3600);
         $this->assertContainerBuilderHasParameter('kunstmaan_node.url_chooser.lazy_increment', 2);
     }
+
+    /**
+     * @group legacy
+     */
+    public function testImprovedRouterConfigDeprecation()
+    {
+        $this->expectDeprecation('Since kunstmaan/node-bundle 7.2: Not setting the "kunstmaan_node.enable_improved_router" config to true is deprecated, it will always be true in 8.0.');
+        $this->container->setParameter('twig.form.resources', []);
+        $this->load();
+    }
 }