[Messenger] Support extending AsMessageHandler attribute

[RobertMe]

Description

The AsMessageHandler attribute has some options which would be pretty common / supplied by numerous locations using the attribute. For example the $bus argument, if you define multiple buses you would most likely want to define this argument for all (or all except one, the default, bus type). Being able to extend the AsMessageHandler would make this a lot simpler.

Currently one can actually extend the class. But this doesn't work as the AttributeAutoconfigurationPass doesn't handle inheritance. Something which it IMO also shouldn't do by default. But in some cases there are attribute where it really makes sense to allow inheritance.

In case this wouldn't be accepted, it is my opinion that the attribute class, i.e. AsMessageHandler should be final, as extending it actually doesn't work / extending it only has the meaning of inheriting the properties, but not the behavior (autoconfiguration) applied to the attribute.

Example

#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::IS_REPEATABLE)]
class AsCommandHandler extends AsMessageHandler
{
    public function __construct(
        ?string $fromTransport = null,
        ?string $handles = null,
        ?string $method = null,
        int $priority = 0,
    ) {
        parent::__construct('command.bus', $fromTransport, $handles, $method, $priority);
    }
}

in combination with

#[AsCommandHandler]
class FooHandler
{
  ...
}

#[AsCommandHandler]
class BarHandler
{
  ...
}

Instead of the current:

#[AsMessageHandler('command.bus')]
class FooHandler
{
  ...
}

#[AsMessageHandler('command.bus')]
class BarHandler
{
  ...
}

(obviously having multiple As...Handler attributes which use different $bus arguments)

[chalasr]

👍 To make it final. Symfony wouldn't be able to handle any extra capacity added to the child attribute so allowing to extend attributes via inheritance would lead to too much confusing situations. For this use case one could use the Autoconfigure attribute instead.

[RobertMe]

Symfony wouldn't be able to handle any extra capacity added to the child attribute so allowing to extend attributes via inheritance would lead to too much confusing situations.

Agreed on this, didn't think about that. But yes, when adding any property (or whatever) to the extended class then the handling "of Symfony" doesn't understand it anyway.

For this use case one could use the Autoconfigure attribute instead.

I'm unfamiliar with that attribute. But after giving it a quick glance I don't think that's a solution? It would only allow me to configure an interface for autoconfiguration (with a tag), so it's not an attribute, nor contains some default configuration. And on top of that using Autoconfigure wouldn't allow for dynamic attributes on the added tags? I.e.: I can't set "fromTransport", or any of the other properties on it, or at least not based on some things of the implementation. As with #[AsMessageHandler] you can set "fromTransport" for which the value is converted in a attribute (/key/value) on the tag. So it isn't the same.

So I'll just have to copy paste class AsMessageHandler fully, rename it, and copy/paste the ->registerAttributeForAutoconfiguration call.

By the way: doesn't your argument against this (and preferring making AsMessageHandler final) apply to Autoconfigure as well? That's also an "open" class, and is extended, by AutoconfigureTag, as a convenience. While the same argument applies: "when one does extend it one can't add properties / functionality as those won't be recognized / don't automatically manifest some functionality". So following your argument Autoconfigure should be final as well (and AutoconfigureTag "can't exist" / can't extend Autoconfigure and requires explicit handling).
So I think that deserves some reconsideration as well then?

[nicolas-grekas]

Each attributes are handled differently.

• registerAttributeForAutoconfiguration matches by exact class name
• Autoconfigure / Autowire match by "instanceof"

There's no one-size-fits-all reasoning.

Maybe registerAttributeForAutoconfiguration could be made to work by "instanceof" - or maybe there are valid reasons to have it as is. I don't remember. What @chalasr describes is still accurate today so if you'd like things to be otherwise, I invite you to have a look at the code and see what could be done about it.

This would be for another issue - not directly related to AsMessageHandler, so this issue if fine closed now.

[nicolas-grekas]

Thinking twice about this, I think the current behavior is legit. Unlike Autowire/Autoconfigure, registerAttributeForAutoconfiguration provides a way to read extra properties. The former don't so they can be inherited without loosing any semantics.

[RobertMe]

@chalasr:

Symfony wouldn't be able to handle any extra capacity added to the child attribute so allowing to extend attributes via inheritance would lead to too much confusing situations

This actually is only "halve" the truth.

symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

Lines 670 to 679 in a3686fb

	$tagAttributes = get_object_vars($attribute);
	$tagAttributes['from_transport'] = $tagAttributes['fromTransport'];
	unset($tagAttributes['fromTransport']);
	if ($reflector instanceof \ReflectionMethod) {
	if (isset($tagAttributes['method'])) {
	throw new LogicException(sprintf('AsMessageHandler attribute cannot declare a method on "%s::%s()".', $reflector->class, $reflector->name));
	}
	$tagAttributes['method'] = $reflector->getName();
	}
	$definition->addTag('messenger.message_handler', $tagAttributes);

In other words: any extra properties one could add to an extended #[AsMessageHandler] would actually just be set as attributes on the tag. So extending AsMessageHandler and adding a public string $foo to it would map to a 'foo' => ... tag attribute. But obviously if one really needs additional logic based on additional attributes that wouldn't work. But at the same time: shouldn't such additional attributes be handled in a compiler pass anyway? Because a tag (and it's attributes) on it's own don't do anything and don't add any logic / behavior. It's the compiler pass which acts on the tag (and optional attributes) to configure the container.

@nicolas-grekas

This would be for another issue - not directly related to AsMessageHandler, so this issue if fine closed now.

Agreed that it would be a generic implementation. But at the same time adding it needs a use case. I guess that creating a PR to change the workings of registerAttributeForAutoconfiguration would also lead to discussions about what the use case would be.

Unlike Autowire/Autoconfigure, registerAttributeForAutoconfiguration provides a way to read extra properties. The former don't so they can be inherited without loosing any semantics.

I'm not sure I understand what you mean. IMO, also based on above comment (any properties being mapped to tag attributes), it would make more sense that registerAttributeForAutoconfiguration would support inheritance (maybe not for all attributes, but either restricted by making the attribute final which blocks it obviously, or based on a boolean param to registerAttributeForAutoconfiguration). This while #[Autowire] and #[Autoconfigurate] can actually be extended (and actually done so in Symfony itself), but added functionality (/added properties) won't work. And yes, the current derived classes / attributes only invoke the base constructor as some form of "helper" / "shortcut" / "currying" so even Symfony itself doesn't need to know about these derived attributes. But for my described use case, for being able to extend #[AsMessageHandler], it would be the same: just creating derived attributes as a form of "helper" / "shortcut".
And while you do name the (technical) differences between support for #[Autowire] / #[Autoconfigure] vs the generic registerAttributeForAutoconfiguration, it's my believe that eventually that's just an implementation detail. So given a proper use case (for which I created this ticket) that implementation detail may be solved.

[GromNaN]

@RobertMe thank you for opening this issue and exposing the use-case. The feature have been implemented in #54365, and #52971 have been reverted.

[RobertMe]

Thank you for this @GromNaN 🎉

Also good to see (in the PR) that Symfony itself can already make use of it, with the workflow listeners.

(And thanks for the ping as well, wouldn't have noticed it otherwise I guess :))