Subscriber method registration bug

[Niflthaena]

Detailed description

Subscriber registration, based on the documentation, sounds very useful and easy to integrate. Its implementation is decidedly less so.
Specifically, the documentation says that any method on a Subscriber whose name begins with 'on' is registered. However, the code is written such that any method containing the text 'on' is caught. (for example, "encodeJson".)
Per src/OrderedListenerProvider.php:131:
if (!in_array($methodName, $proxy->getRegisteredMethods()) && strpos($methodName, 'on') !== false) {

To make matters worse, if the first parameter of any such method does not have a typehint, auto-registration immediately throws a fatal error, rather than simply failing to register.
Per src/OrderedListenerProvider.php:133:
$type = $params[0]->getType()->getName();
getType() returns null, getName() throws a 'member function on null' fatal error.

Context

When integrating with an existing codebase, it would be nice to be able to use some convention (or annotation) to turn manager classes into subscribers. Instead, auto-registration has the above issue, leaving manual registration as the alternative, or refactoring the classes that wish to subscribe to avoid placing an 'n' after an 'o'.

Test concerns

The current automated tests have methods that are not to be registered, but coincidentally these methods do not include the text 'on' anywhere in their name. A test case such "public function doNotAttach_onRegistration" would test for this, and have caught the issue.

Possible implementation

A potential fix would be simply comparing strpos($methodName, 'on') === 0.

[Niflthaena]

I've added a PR here.
It changes the string comparison to catch only methods starting with 'on', not simply containing it.

It intentionally makes no changes to the exacerbating condition of fatal errors on missing typehints, as that is out of my scope of experience with the project (and might be intentional.)

[Crell]

The over-eager "on" parsing is fixed by #8, now merged.

The failure of methods that have no type hint is expected; a method with no type hint cannot be auto-registered because the library has no idea what to register it for. That said, I agree that a more useful error message would be an improvement. Probably the best would be to modify OrderedListenerProvider to check for a missing type, and throw a more specific and helpful exception in that case. (That way it works for trying to register a type-less listener from any place.)

@Niflthaena Do you want to take a crack at it?

[Niflthaena]

I certainly could. It raises some concerns about changing behavior (replacing a Fatal error with an Exception that could accidentally be caught by existing code), but it's more flexible for new development.

I was mostly concerned because the over-eager parsing was leading to uncatchable errors outside my listeners, making it much harder to integrate into existing code. Either issue alone is much less of a problem; bad listeners that are never called is just a performance hit, and writing a listener wrong and getting a development-time error is good regardless of if it's a Fatal or an Exception.

Regardless, if you think the slight risk of breaking changes is acceptable, I'll put in a PR later.

[Crell]

Changing an unclear fatal error into a self-documenting fatal error (what an uncaught exception is) doesn't qualify as a breaking change in my book, so go for it. It should probably have a dedicated exception class.

[Niflthaena]

PR opened. It syncs up some cases that were argument exceptions and some that were fatal errors to make things more readable and consistent. Detailed notes in the pull request.

[Niflthaena]

And done. All flows through the OrderedListenerProvider should throw consistent errors, and only catch the desired functions when parsing.

In the absence of other cleanup concerns, I'm closing this out.