Skip to content

[DependencyInjection] Apply attribute configurator to child classes #54365

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 23, 2024
Merged

[DependencyInjection] Apply attribute configurator to child classes #54365

merged 1 commit into from
Mar 23, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Mar 21, 2024

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.

@carsonbot carsonbot added this to the 7.1 milestone Mar 21, 2024
@GromNaN GromNaN force-pushed the nearest-attribute-configurator branch from 2c896cb to 2390a26 Compare March 21, 2024 11:13
chalasr
chalasr reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible to me. I'd suggest checking our current attribute list to make sure there's no blocker in extending each of them (and make those final if any, or hard-skip them)

@GromNaN
Copy link
Member Author

GromNaN commented Mar 21, 2024 •
edited
Loading

The list of autoconfigured attributes is in FrameworkExtension

  • AsEventListener
  • AsController
  • AsRemoteEventConsumer
  • AsMessageHandler
  • AsTargetedValueResolver
  • AsSchedule
  • AsPeriodicTask
  • AsCronTask
  • Workflow\Attribute\AsAnnounceListener
  • Workflow\Attribute\AsCompletedListener
  • Workflow\Attribute\AsEnterListener
  • Workflow\Attribute\AsEnteredListener
  • Workflow\Attribute\AsGuardListener
  • Workflow\Attribute\AsLeaveListener
  • Workflow\Attribute\AsTransitionListener

@chalasr
Copy link
Member

chalasr commented Mar 22, 2024

The use case for AsMessageHandler makes sense to me (see linked issue) so I'd be fine reverting the change that made it final, likely for another PR.
For the rest of the attributes I'll let other mergers chime in, but to me all of them feel ok-ish to extend.

@GromNaN
Copy link
Member Author

GromNaN commented Mar 22, 2024

It will be also possible to remove the handling for all the Workflow attributes, since it duplicates AsEventListener handler.

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

Lines 1108 to 1129 in bff8e08

$listenerAttributes = [
Workflow\Attribute\AsAnnounceListener::class,
Workflow\Attribute\AsCompletedListener::class,
Workflow\Attribute\AsEnterListener::class,
Workflow\Attribute\AsEnteredListener::class,
Workflow\Attribute\AsGuardListener::class,
Workflow\Attribute\AsLeaveListener::class,
Workflow\Attribute\AsTransitionListener::class,
];
foreach ($listenerAttributes as $attribute) {
$container->registerAttributeForAutoconfiguration($attribute, static function (ChildDefinition $definition, AsEventListener $attribute, \ReflectionClass|\ReflectionMethod $reflector) {
$tagAttributes = get_object_vars($attribute);
if ($reflector instanceof \ReflectionMethod) {
if (isset($tagAttributes['method'])) {
throw new LogicException(sprintf('"%s" attribute cannot declare a method on "%s::%s()".', $attribute::class, $reflector->class, $reflector->name));
}
$tagAttributes['method'] = $reflector->getName();
}
$definition->addTag('kernel.event_listener', $tagAttributes);
});
}

@GromNaN
Copy link
Member Author

GromNaN commented Mar 22, 2024 •
edited
Loading

All the internal attributes can be extended and we already have an internal use-case with Workflow attributes.

Ready for review.

@fabpot fabpot force-pushed the nearest-attribute-configurator branch from b11cb5b to 69dc71b Compare March 23, 2024 07:14
@fabpot
Copy link
Member

fabpot commented Mar 23, 2024

Thank you @GromNaN.

@fabpot fabpot merged commit b427471 into symfony:7.1 Mar 23, 2024
4 of 10 checks passed
@fabpot fabpot mentioned this pull request Mar 23, 2024
Closed
@GromNaN GromNaN deleted the nearest-attribute-configurator branch March 23, 2024 07:28
@GromNaN GromNaN mentioned this pull request Mar 23, 2024
Merged
@GromNaN GromNaN mentioned this pull request Mar 23, 2024
Merged
nicolas-grekas added a commit that referenced this pull request Mar 23, 2024
@nicolas-grekas
feature #54381 [Messenger] Allow extending attribute class `AsMessage…
0b99d83 
…Handler` (GromNaN)

This PR was merged into the 7.1 branch.

Discussion
----------

[Messenger] Allow extending attribute class `AsMessageHandler`

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | -
| License       | MIT

Revert #52971 which made `AsMessageHandler` final.

Discussed in #54365 (comment)

Commits
-------

9479563 Allow extending AsMessageHandler
nicolas-grekas added a commit that referenced this pull request Mar 23, 2024
@nicolas-grekas
minor #54382 [FrameworkBundle] Remove custom handler for Workflow lis…
cd2fc45 
…tener attributes (GromNaN)

This PR was merged into the 7.1 branch.

Discussion
----------

[FrameworkBundle] Remove custom handler for Workflow listener attributes

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

All the `Workflow\Attribute\As*Listener` classes extend `AsEventListener`. Since the attribute handler are now applied to child classes (#54365), it's no longer necessary to declare an attribute handler for each attribute.

Discussed in #54365 (comment)

Commits
-------

db7004c Remove custom handler for Workflow listener attributes
@GromNaN GromNaN mentioned this pull request Mar 23, 2024
Closed
@fabpot fabpot mentioned this pull request May 2, 2024
Merged
@smnandre smnandre mentioned this pull request Oct 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Reviewed
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

[Messenger] Support extending AsMessageHandler attribute
5 participants
@GromNaN @chalasr @fabpot @nicolas-grekas @carsonbot