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.

carsonbot added Status: Needs Review Deprecation labels

Aug 8, 2021

fabpot mentioned this pull request

Aug 8, 2021

[Notifier] Add notify helper in AbstractController#42418

Closed

OskarStark approved these changes

Aug 8, 2021

View reviewed changes

Copy link

Contributor

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 👍

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Aug 8, 2021

OskarStark reviewed

Aug 8, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php OutdatedShow resolvedHide resolved

jschaedl approved these changes

Aug 8, 2021

View reviewed changes

ro0NL reviewed

Aug 8, 2021

View reviewed changes

Copy link

Contributor

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 :')

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.phpShow resolvedHide resolved

nicolas-grekas added this to the5.4 milestone

Aug 8, 2021

derrabus approved these changes

Aug 8, 2021

View reviewed changes

Copy link

Member

wouterj commentedAug 8, 2021

Makes sense, althoughgetDoctrine() 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.

Copy link

Contributor

zairigimad commentedAug 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 force-pushed theabstract-controller-clarification branch from14d7f9c tob182498Compare

August 8, 2021 11:06

chalasr reviewed

Aug 8, 2021

View reviewed changes

Copy link

Member

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 fromAbstractController::get() when the passed id is one ofmessage_bus,messenger.default_bus ordoctrine.

src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php OutdatedShow resolvedHide resolved

jderusse approved these changes

Aug 8, 2021

View reviewed changes

Copy link

Member

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 force-pushed theabstract-controller-clarification branch fromb182498 to832b78eCompare

August 9, 2021 08:28

Copy link

MemberAuthor

fabpot commentedAug 9, 2021

I suggest to trigger a deprecation notice fromAbstractController::get() when the passed id is one ofmessage_bus,messenger.default_bus ordoctrine.

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