-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
Genuine curiosity questions here:
Are they all combined ? |
b81292e
to
093fce4
Compare
#[Route]
attribute
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 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 |
So no "combined aliases" (invoyable alias + name of the action route), ok!
I guess it would be a better DX than not indeed :) |
cdb3dd7
to
7c76ef2
Compare
Done :) |
7c76ef2
to
abf4cf2
Compare
6e81be1
to
749c442
Compare
#[Route]
attribute#[Route]
attribute
Status : Needs Review |
src/Symfony/Component/Routing/Tests/Fixtures/AttributeFixtures/AliasClassController.php
Show resolved
Hide resolved
There was a problem hiding this 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 = [], |
There was a problem hiding this comment.
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.
cae7ff1
to
c750a97
Compare
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)) { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
The attribute could accept an instance of |
Is 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 ? |
I realize my solution suggestion was incorrect as the Incorrect solutionThe class 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 |
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) |
cb75972
to
33a4c1a
Compare
Status: Needs Review |
c47b529
to
4e71cfe
Compare
4e71cfe
to
d2ae097
Compare
Thank you @damienfern. |
While scrolling the Routing documentation, I noticed that we can't configure aliases with the
#[Route]
attribute.With this PR, we can.