Rule idea: validate that method names returned in implementations of EventSubscriberInterface are valid

[stof]

When implementing the EventSubscriberInterface of Symfony, the structure of the array being returned from getSubscribedEvents is already validated by phpstan thanks to the phpdoc return type, (see https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/EventDispatcher/EventSubscriberInterface.php) except for one requirement: the fact that the subscribed listener is the name of a public method of the subscriber class.

It would be great to provide a custom rule comparing those method names to the list of public methods of the class.

[ondrejmirtes]

For more clarity it'd be good to show examples of code that should and should not be flagged by this rule.

[stof]

The phpdoc of the interface shows the 3 kind of structure we can have in the returned array (different keys of the array might use different structures if they have different needs regarding the priority or the number of listeners).

When the method name is a literal string (probably >99.9% of usages, as the return value cannot be dynamic anyway), the rule would report errors in 2 cases:

• when the class does not have a method named like that string literal
• when the method is not public

namespace App;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class BrokenListener implements EventSubscriberInterface
{
    public static function getSubscribedEvents(): array
    {
        return [
            'first_event' => 'nonExistentMethod', // broken because "nonExistentMethod" is not a method of that class
            'second_event' => ['handleEvent', 12], // broken because "handleEvent" is not public
        ];
    }

    protected function handleEvent() {}
}

[stof]

For completeness, phpstan does not currently detect all mistakes in the returned structure when enabling phpstan-symfony because the stubs are overriding the upstream return type. #435 is about fixing it.

[ondrejmirtes]

What's the significance of ['handleEvent', 12]?

[stof]

Register handleEvent is the event listener, with priority 12

I'm quoting the method documentation here (from the link in the opening issue):

The array keys are event names and the value can be:

• The method name to call (priority defaults to 0)
• An array composed of the method name to call and the priority
• An array of arrays composed of the method names to call and respective
  priorities, or 0 if unset

For instance:

• ['eventName' => 'methodName']
• ['eventName' => ['methodName', $priority]]
• ['eventName' => [['methodName1', $priority], ['methodName2']]]

[ondrejmirtes]

All of these are important for the theoretical rule here.

[stof]

Another alternative would be for phpstan to implement a pseudo-type public-method-of<T of object> (name to be bikeshed), which would allow to describe the contract fully in the return type (by replacing string with public-method-of<static>) and have phpstan enforce that.
public-method-of<T of object> would be the union of strings for the names of public methods in the class T.

such pseudo type might be useful for other kinds of APIs where a method name is referenced as a string.

[pkly]

I just wanted to chime in to say it'd be great if this could eventually be added, since we just had this slip through for the first time and the first thought I had was this exact rule.

For the time being wouldn't it be okay to just implement it by looking at symfony docs and checking the logic appropriately from there?

Additionally I wanted to ask if this were made into a PR would it possibly be accepted, as we might work on it internally.

[ondrejmirtes]

The PR would be reviewed and if it was of sufficient quality, it'd be merged.

[stof]

@ondrejmirtes are you talking about the custom rule in this extension or about the proposal for a public-method-of pseudo-type ?

[ondrejmirtes]

Custom rule here is order of magnitude easier so that.