[EventDispatcher] Throw exception when listener method cannot be resolved

[nikophil]

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
License	MIT
Doc PR	not needed

while working on #50687 I've discovered bugs on how the method for listener is resolved:

Given this code:

#[AsEventListener(event: KernelEvents::REQUEST, priority: 5)]
class MyListenerNotInvokable
{
    public function someMethod(RequestEvent $event): void {}
}

A listener on MyListenerNotInvokable::onKernelRequest() is registered, and then an error is dispatched at runtime when the event is dispatched: Error: Call to undefined method App\EventListener\MyListenerNotInvokable::onKernelRequest()

The problem comes from this code: since the method is omitted in tag definition, we "camelize" the event's name. We then try to fallback on __invoke but because it does not exist, the wrong method name is kept.

this PR fixes this behavior by throwing an exception at compile time if neither the camlized method exist, not the __invoke method.

ping @GromNaN

[nicolas-grekas]

This should target 5.4 to me since it's a bugfix.

[nikophil]

@nicolas-grekas I've rebased the PR

[GromNaN]

Thank you for your findings on this bug.

If it does not, we evaluate if one unique public method exists which would take the event as unique parameter (ie: the listener definition above would resolve someMethod() as the listener's method).

This looks like a new feature. Even if there is currently a runtime exception for this situation, I don't think we should support it as a bugfix.
The bugfix should be restricted to throwing an exception on build time when the method (__invoke or onEventName) doesn't exist.

I see situations where the "only public method" will fail and the developers will have to rework their configuration while it would be working previously:

• a public getter or setter is added for DI or testing
• a second event listener is added

For brevity, developers should set the AsEventListener attribute on the method instead of the class.

[nikophil]

This looks like a new feature. Even if there is currently a runtime exception for this situation, I don't think we should support it as a bugfix.
The bugfix should be restricted to throwing an exception on build time when the method (__invoke or onEventName) doesn't exist.

ok let's do this: I'll split the PR and in the bugfix I'll only throw an exception if the resolved method does not exist

I see situations where the "only public method" will fail and the developers will have to rework their configuration while it would be working previously:

a public getter or setter is added for DI or testing
a second event listener is added

For brevity, developers should set the AsEventListener attribut on the method instead of the class.

indeed, there will be a failure in these cases, but for both cases, we don't have any way to resolve the method name, then their listener declaration becomes invalid. And since the error will occur in compile time, I think this is acceptable

[GromNaN]

ok let's do this: I'll split the PR and in the bugfix I'll only throw an exception if the resolved method does not exist

👍🏻 Sounds like a plan.

[nikophil]

For brevity, developers should set the AsEventListener attribut on the method instead of the class.

This is just an idea, but since listeners are callables and not actual services, maybe in 7.0 we should allow #[AsEventListener] to be set on the class only if the class in invokable? This would simplify a lot all this method-resolving stuff, and remove some un-needed magic

[GromNaN]

I wouldn't bother developers with such a restriction. We're not going to deprecate documented way of using this attribute.

[nicolas-grekas]

I'll split the PR and in the bugfix I'll only throw an exception if the resolved method does not exist

Totally. I missed this was a new feature.

This is just an idea, but since listeners are callables and not actual services, maybe in 7.0 we should allow #[AsEventListener] to be set on the class only if the class in invokable? This would simplify a lot all this method-resolving stuff, and remove some un-needed magic

I would advise against this. It'd be needlessly rigid.

[GromNaN]

LGTM, after few changes.

[nikophil]

CI failures seems unrelated

[nicolas-grekas]

One minor thing. Can you please also update the commit+PR messages to make them tell what this does?

[nikophil]

@nicolas-grekas done!

[nicolas-grekas]

Thank you @nikophil.