-
Notifications
You must be signed in to change notification settings - Fork 84
Add support for single action controllers, with __invoke method #645
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
ee10814
to
83b698e
Compare
Looks like PHPStan jobs are broken, not related to my changes |
Friendly ping to @dbu |
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.
thanks for looking into this, we should indeed support __invoke controllers.
i will have a look at the phpstan failure.
@@ -27,12 +27,23 @@ public function onKernelRequest(RequestEvent $event): void | |||
$request = $event->getRequest(); | |||
$controller = $this->controllerResolver->getController($request); | |||
|
|||
if (!is_array($controller) || 2 !== count($controller)) { | |||
if (false === $controller) { |
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.
this seems a bit dangerous to me. what if the $controller
is something weird, like a different kind of array, or some non-array non-object but not false?
should we not add the is_object condition here? that should also avoid the uncertainty below whether method and class are null or set.
if (false === $controller) { | |
if (!is_object($controller) && (!is_array($controller) || 2 !== count($controller))) { |
or we could do an if object, elseif array, else return.
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.
Return type for \Symfony\Component\HttpKernel\Controller\ControllerResolverInterface::getController
is callable|false
, so return values are limited, but you are right, there can be string
for example.
I've changed a logic, now it will work only for object
and array
with two elements
83b698e
to
f270e6f
Compare
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.
great, thanks a lot! i like to be very defensive with these things, you never know what explodes randomly for people...
Nice! Any chance for a release soon? Thanks anyway |
yes, "soon" :-) i hope that we can wrap up #646 and include that in the release. if that does not happen this week, i will release what we have. |
Sure, good luck :) |
we have been quick :-D here you go: https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/releases/tag/3.2.0 |
Thank you |
PR sponsored by AlpinResorts