Skip to content
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

Support invokable controllers in annotation toggle listener #44

Merged

Conversation

krizon
Copy link
Contributor

@krizon krizon commented Jul 21, 2017

Symfony supports invokable controllers. So the FilterControllerEvent:: getController can contain an object instead of an array. This MR adds support for invokable controllers in the (annotation) toggle listener.

$object = new \ReflectionClass($class);
$method = $object->getMethod($controller[1]);
} else {
$object = new \ReflectionClass($controller);
Copy link
Member

@othillo othillo Jul 24, 2017

Choose a reason for hiding this comment

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

this probably doesn't work with closures but we didn't support them anyway.

@othillo othillo requested a review from wjzijderveld July 24, 2017 06:57
@asm89
Copy link
Member

asm89 commented Jul 24, 2017

Does Symfony provide any helpers to figure this out?

@asm89
Copy link
Member

asm89 commented Jul 24, 2017

Hm, a quick git grep 'if (is_array($controller)) {' finds a few references. Some of them do something similar, but I couldn't find a helper after looking quickly.

@krizon
Copy link
Contributor Author

krizon commented Jul 24, 2017

@asm89 no I did research and couldn't find either. There are multiple/different checks but not isolated in a re-usable helper

@othillo
Copy link
Member

othillo commented Jul 24, 2017

You can check if __invoke exists in the class.
(Or check if not instanceof Closure).

@krizon
Copy link
Contributor Author

krizon commented Jul 24, 2017

Yeah that's no problem, I only focussed on invokable controllers because that solves our problem but I'll see if I can add support for closures later today.

@wjzijderveld wjzijderveld merged commit d15c4a2 into qandidate-labs:master Jul 24, 2017
@wjzijderveld
Copy link
Member

Thanks @krizon

The checking for __invoke is already done by Reflection in this case. Maybe we should make the validation a bit stricter, although it won't help with the readability of this code :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants