Skip to content

Clarify goals of AbstractController #42422

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
Aug 9, 2021
Merged

Clarify goals of AbstractController #42422

merged 1 commit into from
Aug 9, 2021

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Aug 8, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? yes
Tickets Refs #42418
License MIT
Doc PR n/a

AbstractController should only be about HTTP-related features and we should not encourage developers to put some other logic in controllers. See #42418 for a discussion about it.

@fabpot fabpot mentioned this pull request Aug 8, 2021
Closed
OskarStark
OskarStark approved these changes Aug 8, 2021
View reviewed changes
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Make sense 👍

ro0NL
ro0NL reviewed Aug 8, 2021
View reviewed changes
Copy link
Contributor

@ro0NL ro0NL left a comment •
edited
Loading

Choose a reason for hiding this comment

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

i'll assume the local service refs (eg. getSubscribedServices) remain available in a protected container like till forever :)

UPGRADE file should be updated :')

@nicolas-grekas nicolas-grekas added this to the 5.4 milestone Aug 8, 2021
@wouterj
Copy link
Member

wouterj commented Aug 8, 2021

Makes sense, although getDoctrine() has been there for so long that we have to update quite some docs I think. Also a friendly ping to @weaverryan as I think this will impact SymfonyCasts as well.

@zairigimad
Copy link
Contributor

zairigimad commented Aug 8, 2021 •
edited
Loading

Makes sense, but Messenger is one of the recently added components in the AbstractControllers, I believe it's not only HTTP related feature, should this part be flagged as deprecated and moved out from the AbstractController?

@fabpot fabpot force-pushed the abstract-controller-clarification branch from 14d7f9c to b182498 Compare August 8, 2021 11:06
chalasr
chalasr reviewed Aug 8, 2021
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I suggest to trigger a deprecation notice from AbstractController::get() when the passed id is one of message_bus, messenger.default_bus or doctrine.

jderusse
jderusse approved these changes Aug 8, 2021
View reviewed changes
Copy link
Member

@jderusse jderusse left a comment

Choose a reason for hiding this comment

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

👍 for cleaning this class

@fabpot fabpot force-pushed the abstract-controller-clarification branch from b182498 to 832b78e Compare August 9, 2021 08:28
@fabpot
Copy link
Member Author

fabpot commented Aug 9, 2021

I suggest to trigger a deprecation notice from AbstractController::get() when the passed id is one of message_bus, messenger.default_bus or doctrine.

I will deprecate get and has in another PR as I don't think these methods make sense as they give access to a limited (and arbitrary?) set of services.

@fabpot fabpot force-pushed the abstract-controller-clarification branch from 832b78e to 42f2f92 Compare August 9, 2021 08:34
@fabpot fabpot force-pushed the abstract-controller-clarification branch from 42f2f92 to f3a6721 Compare August 9, 2021 08:35
@fabpot fabpot merged commit 22ce8f6 into symfony:5.4 Aug 9, 2021
@fabpot fabpot mentioned this pull request Aug 9, 2021
Closed
@fabpot fabpot deleted the abstract-controller-clarification branch August 9, 2021 08:37
@fabpot
Copy link
Member Author

fabpot commented Aug 9, 2021

Follow-up: #42442

This was referenced Nov 5, 2021
Merged
Merged
@TomasVotruba TomasVotruba mentioned this pull request Mar 14, 2022
Merged
@TomasVotruba TomasVotruba mentioned this pull request Mar 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark approved these changes

@ro0NL ro0NL ro0NL left review comments

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

@jderusse jderusse jderusse approved these changes

@derrabus derrabus derrabus approved these changes

@jschaedl jschaedl jschaedl approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Deprecation Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.