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

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

Open
stof opened this issue Mar 19, 2025 · 6 comments

Comments

@stof
Copy link
Contributor

stof commented Mar 19, 2025

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
Copy link
Member

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

@stof
Copy link
Contributor Author

stof commented Mar 19, 2025

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
Copy link
Contributor Author

stof commented Mar 19, 2025

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
Copy link
Member

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

@stof
Copy link
Contributor Author

stof commented Mar 19, 2025

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
Copy link
Member

All of these are important for the theoretical rule here.

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

No branches or pull requests

2 participants