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] Make HandleMessageMiddleware::callHandler protected #50980

Open
wants to merge 1 commit into
base: 7.2
Choose a base branch
from

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Jul 14, 2023

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

I made the following changes:

  • make callHandler protected
  • pass the HandlerDescriptor (can be used to read options from the handler)
  • pass the Envelope (can be used to read more stamps)

This allows people to decorate the HandleMessageMiddleware and intercept the moment the handler is called.

I currently have the following use cases for this in my application:

  1. Every handler can decide if they want to wrap their handling in a transaction. This is done by
    a custom #[WrapInTransaction] attribute. When the attribute is there, we wrap it in a database
    transaction. I know the DoctrineTransactionMiddleware exists in the Symfony Doctrine bridge,
    but that runs as middleware and cannot be toggled individually. It would be possible to modify it
    so that it only runs when the handler has it enabled using an option, but that only works for
    buses with only 1 handler. For an event bus, with multiple handlers, that won't be a solution.

  2. Every handler can decide if they want to ignore certain exceptions that are thrown. This is done
    by a custom #[IgnoreException] attribute. For example the handler uses
    #[IgnoreException(EventNotFoundException::class)]. Whenever that exception is thrown, the
    exception is caught and ignored. The middleware thinks it's executed without issues.

I made the following changes:
- make `callHandler` protected
- pass the HandlerDescriptor (can be used to read options from the handler)
- pass the Envelope (can be used to read more stamps)

This allows people to decorate the `HandleMessageMiddleware` and intercept the moment the handler
is called.

I currently have the following use cases for this in my application:

1) Every handler can decide if they want to wrap their handling in a transaction. This is done by
   a custom `#[WrapInTransaction]` attribute. When the attribute is there, we wrap it in a database
   transaction. I know the `DoctrineTransactionMiddleware` exists in the Symfony Doctrine bridge,
   but that runs as middleware and cannot be toggled individually. It would be possible to modify it
   so that it only runs when the handler has it enabled using an option, but that only works for
   buses with only 1 handler. For an event bus, with multiple handlers, that won't be a solution.

2) Every handler can decide if they want to ignore certain exceptions that are thrown. This is done
   by a custom `#[IgnoreException]` attribute. For example the handler uses
   `#[IgnoreException(EventNotFoundException::class)]`. Whenever that exception is thrown, the
   exception is caught and ignored. The middleware thinks it's executed without issues.
@carsonbot carsonbot added this to the 6.4 milestone Jul 14, 2023
ruudk added a commit to ruudk/symfony that referenced this pull request Jul 17, 2023
The `HandlerDescriptor` class is final and therefore cannot be extended. To make this a bit more
flexible we introduce the `HandlerDescriptorInterface` that is used everywhere.

It also introduces a `.messenger.handler_descriptor_class` parameter that's used in the
MessengerPass. With this parameter it's possible to use a custom implementation of the
`HandlersLocator`.

This is an alternative approach to symfony#50980.
@ro0NL
Copy link
Contributor

ro0NL commented Oct 7, 2023

  1. can be solved with a custom middleware isnt it? (i assume the handler can be obtained somehow)
  2. why not try/catch where needed?

@ruudk
Copy link
Contributor Author

ruudk commented Oct 13, 2023

Sure, this can be solved by a custom middleware. But that means that I will be replacing a big part of Messenger, namely the way it handles messages: HandleMessageMiddleware.php.

That requires me to keep my "fork" middleware, sync with every release.

What I want is an extension point, where it passes a message to more than 1 handlers.

My use case: I want to add extra functionality for event subscribers. I want to know which subscriber failed, and which didn't. Or I want to wrap every subscriber inside its own database transaction.

Currently, not possible as we can only wrap ALL handlers.

@ruudk
Copy link
Contributor Author

ruudk commented Oct 13, 2023

A try/catch inside the subscribers will work, but that requires putting this in place in 1000's of subscribers. Or it requires me to create an abstract subscriber that all subscribers need to extend from.

I want to handle this in a higher level. Abstracted away from what developers will see in their day to day.

@ro0NL
Copy link
Contributor

ro0NL commented Oct 13, 2023

oh i get it, envelope has many handlers.

perhaps the better approach is to dispatch an event, enabling wrapping the handler callable

@ruudk
Copy link
Contributor Author

ruudk commented Oct 13, 2023

@ro0NL Interesting. I checked it but I don't see how that would work. Or how that could help my use case.

I really think there should be an extension point for the execution of the handler.

Or, in an ideal world, there is a dedicated middleware stacks per handler. That would allow wrapping handlers isolated, instead of the current situation where multiple handlers can be called.

@ro0NL
Copy link
Contributor

ro0NL commented Nov 2, 2023

see #52425 :)

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants