Skip to content

[Routing] Allow aliases in #[Route] attribute #58819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

damienfern
Copy link
Contributor

@damienfern damienfern commented Nov 9, 2024

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT

While scrolling the Routing documentation, I noticed that we can't configure aliases with the #[Route] attribute.

With this PR, we can.

#[Route('/path', name: 'action_with_alias', alias: ['alias', 'completely_different_name'])]
public function action()
{
}

@carsonbot carsonbot added this to the 7.2 milestone Nov 9, 2024
@carsonbot carsonbot changed the title [WIP] [Routing] set aliases in Route attribute [Routing] [WIP] set aliases in Route attribute Nov 9, 2024
@damienfern damienfern changed the title [Routing] [WIP] set aliases in Route attribute [WIP] [Routing] Add aliases in Route attribute Nov 9, 2024
@carsonbot carsonbot changed the title [WIP] [Routing] Add aliases in Route attribute [Routing] [WIP] Add aliases in Route attribute Nov 9, 2024
@smnandre
Copy link
Member

smnandre commented Nov 9, 2024

Genuine curiosity questions here:

  • How would this work with aliases on the controller class attribute and others on the method attribute ?
  • Same question if aliases are defined on the groupe (in yaml for instance) and then on the controller ?

Are they all combined ?

@OskarStark OskarStark changed the title [Routing] [WIP] Add aliases in Route attribute [Routing] [WIP] Add aliases in Route attribute Nov 10, 2024
@damienfern damienfern force-pushed the feat/route-alias branch 2 times, most recently from b81292e to 093fce4 Compare November 12, 2024 08:07
@OskarStark OskarStark changed the title [Routing] [WIP] Add aliases in Route attribute [Routing] [WIP] Allow aliases in #[Route] attribute Nov 12, 2024
@damienfern
Copy link
Contributor Author

Genuine curiosity questions here:

* How would this work with aliases on the controller class attribute and others on the method attribute ?

* Same question if aliases are defined on the groupe (in yaml for instance) and then on the controller ?

Are they all combined ?

For now, setting aliases on non invokable controller class does nothing with it. The reason is that aliases from the attribute are added when creating the route (handled by the AttributeClassLoader::addRoute) and it needs either the __invoke method or the method where the attribute is used.

IMO, aliases should not be used on class (unless it is invokable). Setting an alias adds another name to a route and if we use alias on a class, it means that all routes declared in this class will have the same alias. But how the router will know which route should be called when we use features like $this->redirectToRoute('my_alias') ? It might be good to trigger an error if devs tries to set aliases on a class instead of on a method. What do you think ?

@smnandre
Copy link
Member

IMO, aliases should not be used on class (unless it is invokable). Setting an alias adds another name to a route and if we use alias on a class, it means that all routes declared in this class will have the same alias.

So no "combined aliases" (invoyable alias + name of the action route), ok!

But how the router will know which route should be called when we use features like $this->redirectToRoute('my_alias') ? It might be good to trigger an error if devs tries to set aliases on a class instead of on a method. What do you think ?

I guess it would be a better DX than not indeed :)

@damienfern
Copy link
Contributor Author

IMO, aliases should not be used on class (unless it is invokable). Setting an alias adds another name to a route and if we use alias on a class, it means that all routes declared in this class will have the same alias.

So no "combined aliases" (invoyable alias + name of the action route), ok!

But how the router will know which route should be called when we use features like $this->redirectToRoute('my_alias') ? It might be good to trigger an error if devs tries to set aliases on a class instead of on a method. What do you think ?

I guess it would be a better DX than not indeed :)

Done :)

@damienfern damienfern changed the title [Routing] [WIP] Allow aliases in #[Route] attribute [Routing] Allow aliases in #[Route] attribute Nov 19, 2024
@damienfern
Copy link
Contributor Author

Status : Needs Review

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until now, you couldn't add an alias to a route configuration. Aliases are defined independently of the route. This is an inversion of the current configuration logic.

Adding aliases parameter to the #[Route] attribute is equivalent to adding it to the Yaml configuration:

- blog_index:
-    alias: blog_list

  blog_list:
      path: /blog
      # the controller value has the format 'controller_class::method_name'
      controller: App\Controller\BlogController::list
+     aliases: [blog_index]

Features like deprecating a route alias are not supported with this configuration way.

@@ -56,6 +57,7 @@ public function __construct(
?bool $utf8 = null,
?bool $stateless = null,
private ?string $env = null,
private array $aliases = [],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there will generally be only one alias, we can accept string|string[] for the attribute and name it alias. Convert to array in the constructor.

   public function __construct(
       // ...
       array|string $alias = [],
   ) {
       // ...
       $this->aliases = (array) $alias;

All the test examples use alias attribute name, which cause the error "Unknown named parameter$alias`" in tests. It needs to be renamed here.

@stof
Copy link
Member

stof commented Jan 13, 2025

This proposal does not allow configuring the alias as deprecated (once it is a common use case for aliases)

{
if (\is_array($aliases)) {
foreach ($aliases as $a) {
if (!\is_string($a)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to be that defensive here. If we have the appropriate string[] type in phpdoc, we always consider it as being part of our contract and we generally don't add associated runtime checks (which require looping over the array). Projects wanting extra guarantees can use static analysis to get those benefits.

}

/**
* @param list<string>|string $aliases
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using string[] instead of list<string> to be less restrictive on the input. This will be consistent with other methods (and with the constructor where you don't ask for a list) and the code consuming getAliases does not even read the keys so it is fine with any array of strings, not just with lists.

@GromNaN
Copy link
Member

GromNaN commented Jan 13, 2025

This proposal does not allow configuring the alias as deprecated (once it is a common use case for aliases)

The attribute could accept an instance of Symfony\Component\Routing\Alias, or maybe a subclass DeprecatedAlias with the deprecation properties set in the constructor.

@damienfern
Copy link
Contributor Author

This proposal does not allow configuring the alias as deprecated (once it is a common use case for aliases)

The attribute could accept an instance of Symfony\Component\Routing\Alias, or maybe a subclass DeprecatedAlias with the deprecation properties set in the constructor.

Is Symfony\Component\Routing\Aliasreally needed ? According to the doc, aliases can be set by string or deprecated record. So this is what I suggest :

class DeprecatedAlias {
    private string $deprecationMessage;
    
    public function __construct(
        private string $aliasName,
        private string $package,
        private string $version,
        string $message = 'Since %package% %version%: The "%aliasName%" route alias is deprecated. You should stop using it, as it will be removed in the future',
    ){
    }
}

And in Route Attribute :

/**
* @param string|string[]|DeprecatedAlias|DeprecatedAlias[]    $alias        The list of aliases for this route
*/

What do you think ?

@GromNaN
Copy link
Member

GromNaN commented Jan 14, 2025

I realize my solution suggestion was incorrect as the Alias class hold the name of the route to alias, not the name of the alias itself.

Incorrect solution

The class Alias already exists and is used by the RouteCollection.

namespace Symfony\Component\Routing;

use Symfony\Component\Routing\Alias;

class DeprecatedAlias extends Alias
{
    public function __construct(string $id, string $package, string $version, string $message) 
    {
        parent::__construct($id);

        $this->setDeprecated($package, $version, $message);
    }
}

And to use:

#[Route('/path', name: 'route_name', alias: new DeprecatedAlias('legacy_alias', 'acme/package', '10.9', 'Deprecation message'))]
public function action()
{
}

I agree agree with the class you suggest, in the Symfony\Component\Routing\Attribute namespace (even if it's not an attribute class).

@stof
Copy link
Member

stof commented Jan 14, 2025

The class used in the attribute should not extend the Alias class IMO (just like our Route attribute does not reuse the Route class used in the RouteCollection, allowing us to adapt the attribute API separately from the low-level API of the component)

@damienfern
Copy link
Contributor Author

Status: Needs Review

@fabpot
Copy link
Member

fabpot commented Jan 25, 2025

Thank you @damienfern.

@fabpot fabpot merged commit d8b3cc6 into symfony:7.3 Jan 25, 2025
3 of 10 checks passed
@fabpot fabpot mentioned this pull request May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.