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

Messenger depends on EventDispatcher, not PSR-14 #45963

Closed
Crell opened this issue Apr 6, 2022 · 7 comments
Closed

Messenger depends on EventDispatcher, not PSR-14 #45963

Crell opened this issue Apr 6, 2022 · 7 comments

Comments

@Crell
Copy link
Contributor

Crell commented Apr 6, 2022

Symfony version(s) affected

5.4

Description

In the Messenger component, several classes depend on Symfony's EventDispatcherInterface rather than the PSR-14 dispatcher interface. In each case the PSR interface is sufficient for the class's needs, but the current type declaration precludes using Messenger with a non-Symfony PSr-14 dispatcher. This should be corrected to use PSR-14 directly for easier reuse.

Specifically:

  • Worker
  • ConsumesMessagesCommand
  • FailedMessagesRetryCommand
  • SendMessageMiddleware
  • SendFailedMessageForRetryListener

How to reproduce

See the classes in the code.

Possible Solution

No response

Additional Context

No response

/cc @weaverryan

@weaverryan
Copy link
Member

Yea, I don’t see a reason why we needed to use Symfony’s ED contract: PSR is all we need. I likely did this, and just wasn’t thinking.

@derrabus
Copy link
Member

derrabus commented Apr 7, 2022

I have done this change for the Mailer component previously: #42662

Looking at what I had to do there to enable PSR-14, I'm not sure if we can/should ship this kind of change as a bugfix though. Do you need it in 5.4 or would it be enough to develop this as a feature for 6.1 or 6.2?

@derrabus
Copy link
Member

derrabus commented Apr 7, 2022

I did some digging. The two commands you listed register event subscribers through Symfony's own mechanism.

if (null !== $this->resetServicesListener && !$input->getOption('no-reset')) {
$this->eventDispatcher->addSubscriber($this->resetServicesListener);
}
$stopsWhen = [];
if ($limit = $input->getOption('limit')) {
$stopsWhen[] = "processed {$limit} messages";
$this->eventDispatcher->addSubscriber(new StopWorkerOnMessageLimitListener($limit, $this->logger));
}
if ($failureLimit = $input->getOption('failure-limit')) {
$stopsWhen[] = "reached {$failureLimit} failed messages";
$this->eventDispatcher->addSubscriber(new StopWorkerOnFailureLimitListener($failureLimit, $this->logger));
}
if ($memoryLimit = $input->getOption('memory-limit')) {
$stopsWhen[] = "exceeded {$memoryLimit} of memory";
$this->eventDispatcher->addSubscriber(new StopWorkerOnMemoryLimitListener($this->convertToBytes($memoryLimit), $this->logger));
}
if (null !== ($timeLimit = $input->getOption('time-limit'))) {
$stopsWhen[] = "been running for {$timeLimit}s";
$this->eventDispatcher->addSubscriber(new StopWorkerOnTimeLimitListener($timeLimit, $this->logger));
}

Decoupling that is probably possible, but it's not an easy bugfix anymore.

Regarding the other classes, I agree with your assessment. They only call the dispatch method in a PSR-14 compatible manner and the EventDispatcherInterface appears in the constructor signature only. We can simply swap the consumed interface here.

@derrabus
Copy link
Member

derrabus commented Apr 7, 2022

#45967

@weaverryan
Copy link
Member

Wow! Thanks for that fast action! I know from talking with @Crell that the command class isn’t necessary to change: I believe IF he were to use Messenger, it wouldn’t include that class specifically :)

@Crell
Copy link
Contributor Author

Crell commented Apr 7, 2022

The Command class is low priority, I agree. It's the others that are blockers.

As far as version, TYPO3 is currently using some 5.x components, so for as long as we're doing that we'd likely stick to the same for Messenger, should we adopt it. (That's what I am currently researching.) That said, 5.4 also has a lot of dependencies that really should be optional, and are in 6.x, which might push us to 6.0 for that one. I don't know yet.

I think it's a bugfix for 6.0.x at least, and up. It would be better to fix it in 5.4.x if possible as it gives both us and others more flexibility. The non-Command class parts should be a trivial change as the Symfony EventDispatcher implements the PSR interface.

@derrabus
Copy link
Member

derrabus commented Apr 8, 2022

As per #45967 (comment) my change will target 6.1 now. But that should not block you if you're still on 5.4. You can install the symfony/event-dispatcher-contracts package which is really small and build a trivial adapter.

class EventDispatcherAdapter implements \Symfony\Contracts\EventDispatcher\EventDispatcherInterface
{
    private $dispatcher;

    public function __construct(\Psr\EventDispatcher\EventDispatcherInterface $dispatcher)
    {
        $this->dispatcher = $dispatcher;
    }

    public function dispatch(object $event, string $eventName = null): object
    {
        return $this->dispatcher->dispatch($event);
    }
}

If my PR gets merged, you would just drop this adapter (and the symfony/event-dispatcher-contracts dependency) as soon as you upgrade to 6.1.

nicolas-grekas added a commit that referenced this issue Apr 11, 2022
…g events (derrabus)

This PR was merged into the 6.1 branch.

Discussion
----------

[Messenger] Consume a PSR-14 dispatcher for dispatching events

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Part of #45963
| License       | MIT
| Doc PR        | N/A

As `@Crell` said in #45963, requiring the event dispatcher passed to those classes to be an implementation of Symfony's contract was too restrictive. We can easily consume a PSR-14 dispatcher here.

This PR obviously only fixes the low hanging fruit. The commands coming with the messenger component register event subscribers on the dispatcher, in a way that we need the actual EventDispatcher component for.

Commits
-------

f500c7a [Messenger] Consume a PSR-14 dispatcher for dispatching events
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants