8000 [make:listener] use FQCN for kernel events, fix adding unnecessary `#[AsEventListener]` arg by kbond · Pull Request #1687 · symfony/maker-bundle · GitHub
[go: up one dir, main page]
Skip to content

[make:listener] use FQCN for kernel events, fix adding unnecessary #[AsEventListener] arg #1687

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 25, 2025
Merged

[make:listener] use FQCN for kernel events, fix adding unnecessary #[AsEventListener] arg #1687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/Maker/MakeListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,12 @@ public function generate(InputInterface $input, ConsoleStyle $io, Generator $gen
$eventFullClassName = $this->eventRegistry->getEventClassName($event);
$eventClassName = $eventFullClassName ? Str::getShortClassName($eventFullClassName) : null;

if (null !== ($eventConstant = $this->getEventConstant($event))) {
$useStatements->addUseStatement(KernelEvents::class);
$eventName = $eventConstant;
} else {
$eventName = class_exists($event) ? \sprintf('%s::class', $eventClassName) : \sprintf('\'%s\'', $event);
if ($this->getEventConstant($event)) {
$event = $eventFullClassName;
}

$eventName = class_exists($event) ? \sprintf('%s::class', $eventClassName) : \sprintf('\'%s\'', $event);

if (null !== $eventFullClassName) {
$useStatements->addUseStatement($eventFullClassName);
}
Expand Down Expand Up @@ -230,6 +229,7 @@ private function generateListenerClass(InputInterface $input, ConsoleStyle $io,
[
'use_statements' => $useStatements,
'event' => $eventName,
'class_event' => str_ends_with($eventName, '::class'),
'event_arg' => $eventClassName ? \sprintf('%s $event', $eventClassName) : '$event',
'method_name' => class_exists($event) ? Str::asEventMethod($eventClassName) : Str::asEventMethod($event),
]
Expand Down
2 changes: 1 addition & 1 deletion templates/event/Listener.tpl.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

final class <?= $class_name."\n" ?>
{
#[AsEventListener(event: <?= $event ?>)]
#[AsEventListener<?php if (!$class_event): ?>(event: <?= $event ?>)<?php endif ?>]
public function <?= $method_name ?>(<?= $event_arg ?>): void
{
// ...
Expand Down
2 changes: 1 addition & 1 deletion tests/Maker/MakeSubscriberTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function getTestDetails(): \Generator
);

self::assertStringContainsString(
'KernelEvents::REQUEST => \'onKernelRequest\'',
'RequestEvent::class => \'onRequestEvent\'',
file_get_contents($runner->getPath('src/EventSubscriber/FooBarSubscriber.php'))
);
}),
Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/make-listener/tests/EventListener/BarListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

final class BarListener
{
#[AsEventListener(event: RequestEvent::class)]
#[AsEventListener]
public function onRequestEvent(RequestEvent $event): void
Copy link
Member

Choose a reason for hiding this comment

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

Feature creep: can these be invokable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep!

My only hesitation on this: I sometimes have multiple related listeners in the same file (think security or request/response). I don't know this is a good enough reason to not have invokable listeners though.

Copy link
Member

Choose a reason for hiding this comment

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

Let's skip

Copy link
Member

Choose a reason for hiding this comment

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

Ideas for better method names:

  • whenKernelRequest()
  • handleKernelRequest()

or descriptive when possible

  • `beforeRequest()
  • afterException()
  • afterResponse()
  • beforeCommand()
  • afterCommand()
  • beforeLogin() (AUTHENTICATION_SUCCESS)

I quite like the descriptive version

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think this could be generated dynamically? Or some kind of common event map?

Copy link

Choose a reason for hiding this comment

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

descriptive is better.whenKernelRequest does not tell me at what point of the workflow is executed

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking we should go with __invoke() - this would match make:message

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking we should go with __invoke() - this would match make:message

I'm not convinced by __invoke here or even in controllers: I don't want a whole new class for a 2nd listener. Message handlers, like commands, really never have a multi-method use-case

you think this could be generated dynamically? Or some kind of common event map?

event map

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's my argument for __invoke() (this applies to make:controller also):

  1. Saves you from thinking of two names: class & method
  2. Since these makers don't allow adding additional methods, you have to manually add them if desired - renaming __invoke() to something more descriptive at the same time seems easy.
    That being said, I don't feel super strongly. It's easy enough to rename the generated method to invoke and that's what I usually do.

{
// ...
Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/make-listener/tests/EventListener/CustomListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

final class CustomListener
{
#[AsEventListener(event: Generator::class)]
#[AsEventListener]
public function onGenerator(Generator $event): void
{
// ...
Expand Down
5 changes: 2 additions & 3 deletions tests/fixtures/make-listener/tests/EventListener/FooBarListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@

use Symfony\Component\EventDispatcher\Attribute\AsEventListener;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;

final class FooBarListener
{
#[AsEventListener(event: KernelEvents::REQUEST)]
public function onKernelRequest(RequestEvent $event): void
#[AsEventListener]
public function onRequestEvent(RequestEvent $event): void
{
// ...
}
Expand Down
5 changes: 2 additions & 3 deletions tests/fixtures/make-listener/tests/EventListener/FooListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@

use Symfony\Component\EventDispatcher\Attribute\AsEventListener;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;

final class FooListener
{
#[AsEventListener(event: KernelEvents::REQUEST)]
public function onKernelRequest(RequestEvent $event): void
#[AsEventListener]
public function onRequestEvent(RequestEvent $event): void
{
// ...
}
Expand Down
5 changes: 2 additions & 3 deletions tests/fixtures/make-listener/tests/EventSubscriber/FooBarSubscriber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,18 @@

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;

class FooBarSubscriber implements EventSubscriberInterface
{
public function onKernelRequest(RequestEvent $event): void
public function onRequestEvent(RequestEvent $event): void
{
// ...
}

public static function getSubscribedEvents(): array
{
return [
KernelEvents::REQUEST => 'onKernelRequest',
RequestEvent::class => 'onRequestEvent',
];
}
}
5 changes: 2 additions & 3 deletions tests/fixtures/make-listener/tests/EventSubscriber/FooSubscriber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,18 @@

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;

class FooSubscriber implements EventSubscriberInterface
{
public function onKernelRequest(RequestEvent $event): void
public function onRequestEvent(RequestEvent $event): void
{
// ...
}

public static function getSubscribedEvents(): array
{
return [
KernelEvents::REQUEST => 'onKernelRequest',
RequestEvent::class => 'onRequestEvent',
];
}
}
0