Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Already on GitHub?Sign in to your account

Merged

Conversation

@Valmonzo
Copy link
Contributor

QA
Branch?7.1
Bug fix?no
New feature?no
Deprecations?yes
IssuesFix#52898
LicenseMIT

#SymfonyHackDay

valtzu and asrorbekh reacted with thumbs up emojijeremyFreeAgent reacted with eyes emoji
@OskarStarkOskarStark changed the title[Messenger] MakeAsMessageHandler final[Messenger] Make#[AsMessageHandler] finalDec 9, 2023
Copy link
Member

@derrabusderrabus 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?

chalasr, OskarStark, and asrorbekh reacted with thumbs up emoji
@RobertMe
Copy link
Contributor

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) thebus for every usage of the attribute. So personally I would prefer a separate discussion on whether such a feature would be possible, with or withoutAsMessageHandler beingfinal. 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 thatall attribute classes should be madefinal. While when only checking theDependencyInjection\Attribute namespace there are more attribute classesnotfinal than ones which actuallyare final. And as a matter of fact, there are also some attributes which are "open" on purpose as other attributes extend from them (for exampleAutowire which is extended byAutowireCallable,AutowireIterator,AutowireLocator andAutowireServiceClosure). 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 thatall 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 mentionedAutowire* tags where it previously was considered fine to extend the attribute class).

@nicolas-grekasnicolas-grekasforce-pushed thefeat/make-AsMessageHandler-final branch from3ec17f0 tof92e6c3CompareDecember 10, 2023 19:24
@nicolas-grekas
Copy link
Member

Thank you@Valmonzo.

Valmonzo, jeremyFreeAgent, and asrorbekh reacted with heart emoji

@nicolas-grekasnicolas-grekas merged commita3686fb intosymfony:7.1Dec 10, 2023
@ValmonzoValmonzo deleted the feat/make-AsMessageHandler-final branchDecember 11, 2023 08:08
nicolas-grekas added a commit that referenced this pull requestMar 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       | MITRevert#52971 which made `AsMessageHandler` final.Discussed in#54365 (comment)Commits-------9479563 Allow extending AsMessageHandler
@fabpotfabpot mentioned this pull requestMay 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@OskarStarkOskarStarkOskarStark approved these changes

@derrabusderrabusderrabus approved these changes

@chalasrchalasrchalasr approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

[Messenger] Support extendingAsMessageHandler attribute

7 participants

@Valmonzo@RobertMe@nicolas-grekas@OskarStark@derrabus@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp