NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[Messenger] Introduce`HandlerDescriptorInterface`#50998

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

ruudk wants to merge1 commit intosymfony:7.4

base:7.4

Choose a base branch

fromruudk:handler-descriptor-interface

Open

[Messenger] Introduce`HandlerDescriptorInterface`#50998

ruudk wants to merge1 commit intosymfony:7.4fromruudk:handler-descriptor-interface

Conversation

Copy link

Contributor

ruudk commentedJul 17, 2023

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

TheHandlerDescriptor class is final and therefore cannot be extended. To make this a bit more flexible we introduce theHandlerDescriptorInterface 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 theHandlersLocator.

This is an alternative approach to#50980. That PR also describes reasonings for this.

Introduce HandlerDescriptorInterface

a7e15e7

The `HandlerDescriptor` class is final and therefore cannot be extended. To make this a bit moreflexible we introduce the `HandlerDescriptorInterface` that is used everywhere.It also introduces a `.messenger.handler_descriptor_class` parameter that's used in theMessengerPass. With this parameter it's possible to use a custom implementation of the`HandlersLocator`.This is an alternative approach tosymfony#50980.

carsonbot added Status: Needs Review Feature Messenger labels

Jul 17, 2023

carsonbot added this to the6.4 milestone

Jul 17, 2023

Copy link

ContributorAuthor

ruudk commentedJul 17, 2023

Before I fix the failing tests, I'd like to know if this would be considered for merge or not.

Copy link

ContributorAuthor

ruudk commentedAug 3, 2023

@OskarStark what do you think about this?

Copy link

Member

derrabus commentedAug 3, 2023

It also introduces a.messenger.handler_descriptor_class parameter that's used in the MessengerPass.

We've removed this kind of parameter from our codebase and I don't think we want to start this again. A big issue with that class parameter is that it only works if the custom class uses the exact same constructor signature as the default implementation.

With this parameter it's possible to use a custom implementation of theHandlersLocator.

You could also just replace or decorate the service.

Copy link

ContributorAuthor

ruudk commentedAug 3, 2023

Ahh, didn't think of decorating! Will update the PR to remove the parameter.

Copy link

ContributorAuthor

ruudk commentedAug 3, 2023

@derrabus But how to decorate something like this:

symfony/src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

Lines 166 to 169 inbb7a7dd

	foreach ($handlersas$handler) {
	$definitions[$definitionId ='.messenger.handler_descriptor.'.ContainerBuilder::hash($bus.':'.$message.':'.$handler[0])] = (newDefinition(HandlerDescriptor::class))->setArguments([newReference($handler[0]),$handler[1]]);
	$handlerDescriptors[] =newReference($definitionId);
	}