[Routing] Add support for aliasing routes #38464

Lctrs · 2020-10-07T16:03:09Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets	Fix #6088
License	MIT
Doc PR	None yet

Continuation of #38389

Lctrs · 2020-10-07T16:05:14Z

@stof Still WIP but I've made some changes to make deprecated aliases a thing as pointed out in #38389 (comment). WDYT ?

src/Symfony/Component/Routing/RouteCollection.php

Lctrs · 2020-10-23T21:13:11Z

Ready for review finally.

src/Symfony/Component/Routing/Generator/CompiledUrlGenerator.php

src/Symfony/Component/Routing/Alias.php

src/Symfony/Component/Routing/RouteCollection.php

src/Symfony/Component/Routing/Generator/Dumper/CompiledUrlGeneratorDumper.php

src/Symfony/Component/Routing/Alias.php

src/Symfony/Component/Routing/Loader/Configurator/AliasConfigurator.php

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

src/Symfony/Component/Routing/Generator/CompiledUrlGenerator.php

Lctrs · 2020-12-30T00:37:49Z

@jderusse I adressed all your comments except the ones where I left a comment indicating otherwise.

haivala · 2021-09-21T21:21:34Z

Hope this gets merged!

Lctrs · 2021-09-21T22:51:13Z

I don't promise anything but I'll try to come back to this in the next weeks.

Lctrs · 2021-10-17T13:23:42Z

@jderusse @derrabus I have rebased the PR on top of 5.4 branch and adressed all your comments.

I tested this on a local test project and it worked as I would expect.

Do you have any more reviews to make ?

Lctrs · 2021-10-17T13:43:00Z

AppVeyor's failure is unrelated.

jderusse

Thank you for moving this PR.
last few comment for my side.

src/Symfony/Component/Routing/Generator/CompiledUrlGenerator.php

src/Symfony/Component/Routing/Loader/XmlFileLoader.php

jderusse · 2021-10-17T13:44:18Z

src/Symfony/Component/Routing/Loader/Configurator/RoutingConfigurator.php

+    final public function alias(string $name, string $alias): AliasConfigurator
+    {
+        return new AliasConfigurator(
+            $this->collection->addAlias($name, $alias)
+        );
+    }


If we use the collection to build AliasConfigurator, I wonder if this method should'nt be part of CollectionConfigurator instead of RoutingConfigurator

IMHO, from DX point of view, I would expect to be able to define alias directly from a RoutingConfigurator instance, to match DI's side.

src/Symfony/Component/Routing/Alias.php

nicolas-grekas

Thanks, this looks really nice. Here are some nitpicking.

nicolas-grekas · 2021-11-02T14:00:20Z

src/Symfony/Component/Routing/Alias.php

+
+use Symfony\Component\Routing\Exception\InvalidArgumentException;
+
+final class Alias


I think we should remove the final here

nicolas-grekas · 2021-11-02T14:01:14Z

src/Symfony/Component/Routing/Alias.php

+        $this->id = $id;
+    }
+
+    public function redirect(string $id): self


withId could be less confusing, this doesn't redirect in the http meaning

nicolas-grekas · 2021-11-02T14:01:39Z

src/Symfony/Component/Routing/Alias.php

+
+    /**
+     * Whether this alias is deprecated, that means it should not be referenced
+     * anymore.


to be merged with the previous line

nicolas-grekas · 2021-11-02T14:02:51Z

src/Symfony/Component/Routing/Alias.php

+        $this->deprecation = [
+            'package' => $package,
+            'version' => $version,
+            'message' => $message ?: 'The "%alias%" route alias is deprecated. You should stop using it, as it will be removed in the future.',


%alias_id%?

nicolas-grekas · 2021-11-02T14:03:28Z

src/Symfony/Component/Routing/Alias.php

+
+    public function isDeprecated(): bool
+    {
+        return [] !== $this->deprecation;


return (bool) $this->deprecation; if we keep taking inspiration from Alias in DI

nicolas-grekas · 2021-11-02T14:11:38Z

src/Symfony/Component/Routing/Loader/Configurator/AliasConfigurator.php

+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\Routing\Alias;
+
+final class AliasConfigurator


should not be final

nicolas-grekas · 2021-11-02T14:13:36Z

src/Symfony/Component/Routing/Loader/Configurator/RoutingConfigurator.php

@@ -60,6 +60,13 @@ final public function collection(string $name = ''): CollectionConfigurator
        return new CollectionConfigurator($this->collection, $name);
    }

+    final public function alias(string $name, string $alias): AliasConfigurator


shouldn't this be on AddTrait?

nicolas-grekas · 2021-11-02T14:14:47Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

+    /**
+     * @param array  $config A resource config
+     * @param string $name   The config key
+     * @param string $path   The loaded file path


all 3 above lines could be removed imho

nicolas-grekas · 2021-11-02T14:15:50Z

src/Symfony/Component/Routing/Loader/YamlFileLoader.php

+     * @throws \InvalidArgumentException If one of the provided config keys is not supported,
+     *                                   something is missing or the combination is nonsense
+     */
+    private function validateAlias($config, string $name, string $path): void


array $config

nicolas-grekas · 2021-11-02T14:18:32Z

src/Symfony/Component/Routing/Router.php

            }
-            $this->generator = new $this->options['generator_class']($routes, $this->context, $this->logger, $this->defaultLocale);
+            $this->generator = new $this->options['generator_class'](


please keep this on one line

stof · 2021-11-02T15:21:21Z

src/Symfony/Component/Routing/Generator/Dumper/CompiledUrlGeneratorDumper.php

+
+                $deprecations[] = $deprecation;
+
+                trigger_deprecation($deprecation['package'], $deprecation['version'], $deprecation['message']);


This is indeed wrong. Dumping the URL generator should not trigger a deprecation. It should only be triggered when generating the url.
The only compile-time deprecation should be when a non-deprecated alias targets a deprecated one.

Lctrs · 2021-11-02T20:22:22Z

@nicolas-grekas @stof I believe I adressed all your comments.

nicolas-grekas

One last round of review

nicolas-grekas · 2021-11-03T08:46:33Z

src/Symfony/Component/Routing/RouteCollection.php

@@ -116,6 +126,10 @@ public function all()
     */
    public function get(string $name)
    {
+        while ($this->hasAlias($name)) {
+            $name = $this->getAlias($name)->getId();


shouldn't this loop detect circular refs + trigger a deprecation?
why do we need this logic btw? don't we always resolve the aliases before calling the method?
alternatively, shouldn't we move the logic in CompiledUrlGeneratorDumper and UrlGenerator here?

src/Symfony/Component/Routing/Alias.php

nicolas-grekas · 2021-11-03T08:48:31Z

src/Symfony/Component/Routing/Loader/Configurator/Traits/AddTrait.php

@@ -42,6 +43,13 @@ public function add(string $name, $path): RouteConfigurator
        return new RouteConfigurator($this->collection, $route, $this->name, $parentConfigurator, $this->prefixes);
    }

+    final public function alias(string $name, string $alias): AliasConfigurator


no need for final

nicolas-grekas · 2021-11-03T08:48:40Z

src/Symfony/Component/Routing/Loader/Configurator/Traits/AddTrait.php

@@ -42,6 +43,13 @@ public function add(string $name, $path): RouteConfigurator
        return new RouteConfigurator($this->collection, $route, $this->name, $parentConfigurator, $this->prefixes);
    }

+    final public function alias(string $name, string $alias): AliasConfigurator
+    {
+        return new AliasConfigurator(


can be on one line

nicolas-grekas · 2021-11-03T08:50:00Z

src/Symfony/Component/Routing/Generator/CompiledUrlGenerator.php

@@ -50,7 +50,11 @@ public function generate(string $name, array $parameters = [], int $referenceTyp
            throw new RouteNotFoundException(sprintf('Unable to generate a URL for the named route "%s" as such route does not exist.', $name));
        }

-        [$variables, $defaults, $requirements, $tokens, $hostTokens, $requiredSchemes] = $this->compiledRoutes[$name];
+        [$variables, $defaults, $requirements, $tokens, $hostTokens, $requiredSchemes, $deprecations] = $this->compiledRoutes[$name];


it would be safer to add + [6 => []] at the end of the expression in case the compiled routes come from a previous version of Routing. This might help with upgrading.

Lctrs · 2021-11-03T20:29:30Z

@nicolas-grekas all done.

nicolas-grekas

Really cool :)

nicolas-grekas · 2021-11-04T14:19:50Z

src/Symfony/Component/Routing/RouteCollection.php

@@ -118,6 +129,24 @@ public function all()
     */
    public function get(string $name)
    {
+        $visited = [];
+        while (null !== $alias = $this->aliases[$name] ?? null) {


I moved this loop from UrlGenerator to RouteCollection

nicolas-grekas · 2021-11-04T14:20:29Z

src/Symfony/Component/Routing/RouteCollection.php

+        return $this->aliases;
+    }
+
+    public function getAlias(string $name): ?Alias


I removed hasAlias() and made getAlias() nullable instead, for consistency with get()

nicolas-grekas · 2021-11-04T14:25:24Z

Thank you @Lctrs.

Lctrs · 2021-11-04T15:08:36Z

Thank you for taking care of the last polishing @nicolas-grekas

matks · 2021-11-16T14:37:36Z

Thank you for the great feature

carsonbot added Status: Needs Review Routing Feature Deprecation labels Oct 7, 2020

Lctrs mentioned this pull request Oct 7, 2020

[Routing] Add support for aliasing routes #38389

Closed

PierreRambaud reviewed Oct 8, 2020

View reviewed changes

src/Symfony/Component/Routing/RouteCollection.php Outdated Show resolved Hide resolved

nicolas-grekas added this to the 5.x milestone Oct 12, 2020

Lctrs changed the title ~~[WIP][Routing] Add support for aliasing routes~~ [Routing] Add support for aliasing routes Oct 23, 2020

Lctrs commented Oct 23, 2020

View reviewed changes

src/Symfony/Component/Routing/Generator/CompiledUrlGenerator.php Outdated Show resolved Hide resolved

derrabus reviewed Oct 23, 2020

View reviewed changes

src/Symfony/Component/Routing/Alias.php Outdated Show resolved Hide resolved

derrabus reviewed Oct 23, 2020

View reviewed changes

src/Symfony/Component/Routing/RouteCollection.php Outdated Show resolved Hide resolved

src/Symfony/Component/Routing/RouteCollection.php Outdated Show resolved Hide resolved

src/Symfony/Component/Routing/Generator/Dumper/CompiledUrlGeneratorDumper.php Show resolved Hide resolved

jderusse reviewed Dec 17, 2020

View reviewed changes

Lctrs marked this pull request as draft September 21, 2021 22:51

Lctrs marked this pull request as ready for review October 17, 2021 13:21

jderusse reviewed Oct 17, 2021

View reviewed changes

derrabus approved these changes Oct 27, 2021

View reviewed changes

src/Symfony/Component/Routing/Alias.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 27, 2021

nicolas-grekas reviewed Nov 2, 2021

View reviewed changes

stof reviewed Nov 2, 2021

View reviewed changes

nicolas-grekas reviewed Nov 3, 2021

View reviewed changes

[Routing] Add support for aliasing routes

43575a1

nicolas-grekas approved these changes Nov 4, 2021

View reviewed changes

nicolas-grekas mentioned this pull request Nov 4, 2021

[Routing] Add support for aliasing routes symfony/symfony-docs#16070

Closed

nicolas-grekas merged commit 873e8ab into symfony:5.4 Nov 4, 2021

Lctrs deleted the feature/route-alias branch November 4, 2021 14:47

This was referenced Nov 5, 2021

Release v5.4.0-BETA1 #43937

Merged

Release v6.0.0-BETA1 #43939

Merged

matks mentioned this pull request Nov 16, 2021

[SF Migration] Be able to deprecate a Symfony route PrestaShop/PrestaShop#12432

Closed

HypeMC mentioned this pull request Nov 29, 2023

[Routing] Prefix feature for i18n routes is broken #52802

Closed


		use Symfony\Component\Routing\Exception\InvalidArgumentException;

		final class Alias


		$deprecations[] = $deprecation;

		trigger_deprecation($deprecation['package'], $deprecation['version'], $deprecation['message']);

[Routing] Add support for aliasing routes #38464

[Routing] Add support for aliasing routes #38464

Conversation

Lctrs commented Oct 7, 2020 • edited Loading

Lctrs commented Oct 7, 2020

Lctrs commented Oct 23, 2020

Lctrs commented Dec 30, 2020

haivala commented Sep 21, 2021

Lctrs commented Sep 21, 2021

Lctrs commented Oct 17, 2021

Lctrs commented Oct 17, 2021

jderusse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lctrs commented Nov 2, 2021

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lctrs commented Nov 3, 2021

nicolas-grekas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Nov 4, 2021

Lctrs commented Nov 4, 2021

matks commented Nov 16, 2021

Lctrs commented Oct 7, 2020 •

edited

Loading

nicolas-grekas Nov 2, 2021 •

edited

Loading

nicolas-grekas Nov 2, 2021 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading