[Messenger] Support extending `AsMessageHandler` attribute #52898

RobertMe · 2023-12-05T11:34:59Z

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)

The text was updated successfully, but these errors were encountered:

chalasr · 2023-12-09T13:22:42Z

👍 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 · 2023-12-09T21:22:29Z

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 · 2023-12-10T19:35:32Z

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 · 2023-12-10T20:00:57Z

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 · 2023-12-11T21:36:30Z

@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.

…child classes (GromNaN) This PR was squashed before being merged into the 7.1 branch. Discussion ---------- [DependencyInjection] Apply attribute configurator to child classes | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix #52898 | License | MIT This allows extending attribute classes registered for autoconfiguration. Use-case: Share configuration between several classes with pre-configured attribute classes. As described in #52898 (bus name for `AsMessageHandler`, schedule name for `AsCronTask` and `AsPeriodicTask`) The child-class attribute is handled by the same function as it's parent class that is registered for autoconfiguration. This means any additional property will be ignored (unless supported, which could be new feature for the `AsTaggedItem` attribute configurator). If there is a configurator for the child class, the configurator for the parent class is not called. Commits ------- 69dc71b [DependencyInjection] Apply attribute configurator to child classes

GromNaN · 2024-03-23T11:13:14Z

@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 · 2024-03-23T17:38:11Z

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 :))

carsonbot added the Messenger label Dec 5, 2023

GromNaN added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Dec 5, 2023

Valmonzo mentioned this issue Dec 9, 2023

[Messenger] Make #[AsMessageHandler] final #52971

Merged

nicolas-grekas closed this as completed in a3686fb Dec 10, 2023

GromNaN mentioned this issue Mar 21, 2024

[DependencyInjection] Apply attribute configurator to child classes #54365

Merged

GromNaN mentioned this issue Mar 27, 2024

[DependencyInjection] Apply attribute configurator to child classes symfony/symfony-docs#19700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Support extending `AsMessageHandler` attribute #52898

[Messenger] Support extending `AsMessageHandler` attribute #52898

RobertMe commented Dec 5, 2023

chalasr commented Dec 9, 2023

RobertMe commented Dec 9, 2023

nicolas-grekas commented Dec 10, 2023

nicolas-grekas commented Dec 10, 2023

RobertMe commented Dec 11, 2023

GromNaN commented Mar 23, 2024

RobertMe commented Mar 23, 2024

[Messenger] Support extending AsMessageHandler attribute #52898

[Messenger] Support extending AsMessageHandler attribute #52898

Comments

RobertMe commented Dec 5, 2023

Description

Example

chalasr commented Dec 9, 2023

RobertMe commented Dec 9, 2023

nicolas-grekas commented Dec 10, 2023

nicolas-grekas commented Dec 10, 2023

RobertMe commented Dec 11, 2023

GromNaN commented Mar 23, 2024

RobertMe commented Mar 23, 2024

[Messenger] Support extending `AsMessageHandler` attribute #52898

[Messenger] Support extending `AsMessageHandler` attribute #52898