Skip to content

[Messenger] Make #[AsMessageHandler] final #52971

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

Merged

Conversation

Valmonzo
Copy link
Contributor

@Valmonzo Valmonzo commented Dec 9, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? no
Deprecations? yes
Issues Fix #52898
License MIT

#SymfonyHackDay

@OskarStark OskarStark changed the title [Messenger] Make AsMessageHandler final [Messenger] Make #[AsMessageHandler] final Dec 9, 2023
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Let's also update UPGRADE-7.1.md?

@RobertMe
Copy link
Contributor

RobertMe commented Dec 9, 2023

Issues: Fix #52898

I don't think that's really the case. This "fix" changes something which didn't work into something which still doesn't work, but just for a different reason. So while it does relate to my feature request, it doesn't fix it. And I'm not even sure that the acceptation of this change would also answer my feature request. It does close of one possibility to support / implement the feature request. But it doesn't void the feature request to be able to "shortcut" #[AsMessageHandler] so not to have to repeat (for example) the bus for every usage of the attribute. So personally I would prefer a separate discussion on whether such a feature would be possible, with or without AsMessageHandler being final. While such discussion would be ended "automatically" when the fixes / closes "tag" is applied.

Furthermore, for the reasons given by @chalasr in #52898 it is my believe that all attribute classes should be made final. While when only checking the DependencyInjection\Attribute namespace there are more attribute classes not final than ones which actually are final. And as a matter of fact, there are also some attributes which are "open" on purpose as other attributes extend from them (for example Autowire which is extended by AutowireCallable, AutowireIterator, AutowireLocator and AutowireServiceClosure). So while I understand the reasoning to make attribute classes final (as extending them and adding extra properties / functionality doesn't magically work), it would be my believe that all attribute classes should be final.

So to conclude it is my believe that Symfony should have some guideline / "code style" on whether (all) attribute classes must always be final, or whether they may or should be open. And in that discussion the point made by @chalasr obviously should be taken into consideration (but also that fact that said argument is already "violated" by for example all the mentioned Autowire* tags where it previously was considered fine to extend the attribute class).

@nicolas-grekas nicolas-grekas force-pushed the feat/make-AsMessageHandler-final branch from 3ec17f0 to f92e6c3 Compare December 10, 2023 19:24
@nicolas-grekas
Copy link
Member

Thank you @Valmonzo.

@nicolas-grekas nicolas-grekas merged commit a3686fb into symfony:7.1 Dec 10, 2023
@Valmonzo Valmonzo deleted the feat/make-AsMessageHandler-final branch December 11, 2023 08:08
nicolas-grekas added a commit that referenced this pull request Mar 23, 2024
…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
@fabpot fabpot mentioned this pull request May 2, 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.

[Messenger] Support extending AsMessageHandler attribute
7 participants