Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier] Add notify helper in AbstractController#42418
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
derrabus commentedAug 7, 2021
Good idea, please add tests. 🙂 |
FabienPapet commentedAug 7, 2021
That would be awesome ! |
fabpot commentedAug 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm 👎 as we can now easily inject services in controller methods. The code is more explicit and as short (and there is then no need to extend the namespaceApp\Controller;useSymfony\Component\Notifier\Notification\Notification;useSymfony\Component\Notifier\NotifierInterface;class SandboxController{/** * @Route("/onboarding", name="onboarding") */publicfunction__invoke(NotifierInterface$notifier):Response {$notification =newNotification('Welcome Sma <3', ['chat/slack']);$notifier->notify($notification);// stuff }} |
OskarStark commentedAug 7, 2021
I agree with Fabien here, personally I tend to not use AbstractController anymore, as it makes clear the real dependencies of the class/controller. |
chalasr commentedAug 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The goal of |
OskarStark commentedAug 7, 2021
No hard opinion on this 👍🏻 |
ismail1432 commentedAug 7, 2021
Thank you for your feedback.
Many helpers in the Abstract can be injected as well but are stil available via shortcut (Twig, Messenger...) or can be use without the Abstract (throw a notfound or deny access...) Even I personally don't extends the AbstractController many developers and newcomers does. Some of them are not comfortable with dependency injection via Interface. The cons arguments are relevant but I think this feature is a nice to have and doesn't have bad impact |
wouterj commentedAug 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi! I'm also leaning towards 👎 here. I think the controller helper methods function to reduce boilerplate of things commonly done inside controllers. With the exception of |
derrabus commentedAug 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Same. I have stopped using that class years ago and when I give trainings, I usually recommend not to use it. But I acknowledge that it is convenient, especially for new users, to use If developers should avoid using |
OskarStark commentedAug 8, 2021
We have it as starting point and the most common use cases and we should keep it. The question is, is notifier a common/basic use case? For me not, but I am ok to add this. WDYT@fabpot ? |
fabpot commentedAug 8, 2021
Thanks for the discussion. I'm now definitely against merging this PR:
I've submitted#42422 to take into account our discussion. |
ismail1432 commentedAug 8, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the discussion, now the role of the AbstractController is clear for all of us. 👍🏽 |
This PR was merged into the 5.4 branch.Discussion----------Clarify goals of AbstractController| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | no| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | yes <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets | Refs#42418 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License | MIT| Doc PR | n/aAbstractController 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.Commits-------f3a6721 Clarify goals of AbstractController
As the Notifier become more popular and is not experimental anymore, I propose to add a shortcut in the AbstractController.
I would add tests if you're agree with this feature.