Skip to content
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

Clarify goals of AbstractController #42422

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

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 👍

Copy link
Contributor

@ro0NL ro0NL left a comment

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

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

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

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 deleted the abstract-controller-clarification branch August 9, 2021 08:37
@fabpot
Copy link
Member Author

fabpot commented Aug 9, 2021

Follow-up: #42442

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.

None yet