use proper return types in ErrorHandler and ArgumentResolver #32143
Conversation
src/Symfony/Component/HttpKernel/Controller/ArgumentValueResolverInterface.php
Show resolved
Hide resolved
| return $name; | ||
| } | ||
| if (!$function instanceof \ReflectionMethod) { | ||
| return; |
There was a problem hiding this comment.
you cannot have a function using self or parent. so this case cannot happen. I refactored the code to make more sense.
| @@ -55,14 +55,16 @@ public function getArguments(Request $request, $controller) | |||
|
|
|||
| $resolved = $resolver->resolve($request, $metadata); | |||
|
|
|||
| if (!$resolved instanceof \Generator) { | |||
| throw new \InvalidArgumentException(sprintf('%s::resolve() must yield at least one value.', \get_class($resolver))); | |||
There was a problem hiding this comment.
checking for Generator does not achieve the intended goal: having at least one yielded value.
I can do return; yield $value; which would return a generator but not yield anything.
There was a problem hiding this comment.
I don't see a return type on the interface, so this would allow non-iterable return types as well. Perhaps the interface should receive a return type to enforce this (which is \Generator at the moment, iterable after this PR) in 5.0? Could do with a typecheck on $resolved
Another solution would be to allow a return $value in the resolvers as generators are pretty much only required for an array structure being inserted as multiple arguments.
There was a problem hiding this comment.
There are several different topics:
- adding real return types: I would also prefer this. But not sure if we will do that in SF because it's a hard break for all implementations (even with return type variance in PHP 7.4). So that is a different topic.
- Validating returned type: Could make sense. But normally we do not do that for every method as we would just need to check types everywhere. So if you return something non-iterable and violate the interface, the code will just break. No real need to validate that.
- Changing the return type: I also think instead of forcing a iterable/Generator everywhere, it should allow only returning a single value (which is the common need). For variadic arguments, there could just be an explicit
VariadicArgumentValueclass that you can return to resolve to several arguments. I think that's more clear and clean. Currently you can yield several values for non-variadic arguments which makes the ArgumentResolver behave very strange. But that's also a different topic. If you argree with something like this, feel free to create an issue/PR to keep track of that.
00fda63 to
2f9121b
Compare
| return $parent->name; | ||
| if ($function instanceof \ReflectionMethod) { | ||
| $lcName = strtolower($name); | ||
| switch ($lcName) { |
There was a problem hiding this comment.
I'd suggest to inline the strtolower call because we don't need that value anywhere else.
| switch ($lcName) { | |
| switch (strtolower($name)) { |
There was a problem hiding this comment.
Yes seems fine, I just kept the variable because it was there before making merging easier.
|
Thank you @Tobion. |
…lver (Tobion) This PR was merged into the 4.4 branch. Discussion ---------- use proper return types in ErrorHandler and ArgumentResolver | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | tiny | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | | License | MIT | Doc PR | Found those things while reviewing #31996 which missed some return types due to using `return` instead of `return null`. It's part of fixing #17201 (due to #10717). See also #30869 that somebody was stumbling over. Commits ------- 2f9121b use proper return types in ErrorHandler and ArgumentResolver
Found those things while reviewing #31996 which missed some return types due to using
returninstead ofreturn null.It's part of fixing #17201 (due to #10717). See also #30869 that somebody was stumbling over.