[Routing] Convert backed enums to their value when generating default routes #49206
Conversation
|
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
|
Hey! Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "6.3 for features". Cheers! Carsonbot |
|
Please add tests covering that to avoid regressions. |
| @@ -205,7 +205,8 @@ protected function addRoute(RouteCollection $collection, object $annot, array $g | |||
| } | |||
| foreach ($paths as $locale => $path) { | |||
| if (preg_match(sprintf('/\{%s(?:<.*?>)?\}/', preg_quote($param->name)), $path)) { | |||
| $defaults[$param->name] = $param->getDefaultValue(); | |||
| $defaultValue = $param->getDefaultValue(); | |||
| $defaults[$param->name] = $defaultValue instanceof \BackedEnum ? $defaultValue->value : $defaultValue; | |||
There was a problem hiding this comment.
We have rejected this kind of changes in the past. Please call ->value on your enum case yourself if you need the backing value.
There was a problem hiding this comment.
I can't do that in my own code, as stated in #49203, if I set a default value in my function is has to be an Enum, but the router asks for a string.
There was a problem hiding this comment.
To be honest, to me it's more of a bug rather than a feature: as long as Enum is supported for route generation, the behavior that crashes in my issue (setting a default Enum in function parameters) is a bug. I understand that it's easier to port as a new feature but really this should not be a userland problem.
A lot of it still stems from php/php-src#8825 where Enums don't get their __toString method.
There was a problem hiding this comment.
@derrabus this code path is a place where the loader guesses the default based on the controller default value.
In the associated issue, I explicitly asked to change only that place and not adding support for using enums directly in the Route attribute.
There was a problem hiding this comment.
The only other solution would be to make that guesser skip any default value that is not a scalar (to avoid generating an invalid default). But this would make DX worse.
There was a problem hiding this comment.
make that guesser skip any default value that is not a scalar
shouldn't we do this?
$defaults[$param->name] = match (true) {
$defaultValue instanceof \BackedEnum => $defaultValue->value,
$defaultValue instanceof \Stringable => (string) $defaultValue,
\is_object($defaultValue) => null,
default => $defaultValue,
};There was a problem hiding this comment.
I don't think Stringable objects should be casted to create a default. The controller would then receive the string value from the request attributes and not the object that it has a default value.
And having null as default value would be a bad idea as well, as that might not be acceptable by the controller (a null default in the route is not the same than no default)
There was a problem hiding this comment.
We should then just skip objects (and non scalars basically)?
if (\is_scalar($defaultValue = $param->getDefaultValue()) {
$defaults[$param->name] = $defaultValue;
} elseif ($defaultValue instanceof \BackedEnum) {
$defaults[$param->name] = $defaultValue->value;
}There was a problem hiding this comment.
That's probably better than guessing an invalid default, especially as we don't fail during loading for such case but only during route generation later.
There was a problem hiding this comment.
I'll update the code and make some test cases 👍
|
@florentdestremau can you add tests to make this PR ready ? |
|
Yes, I'll spend some time this weekend I hope. |
|
hi @stof , I can't find a good place to start for the tests or some existing ones that are similar. Could you point me in the right direction? |
|
Closing in favor of #50031 |
This is a working update to allow usage of enums as php defaults in controllers.