Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

ismail1432
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR symfony/symfony-docs#...

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.

namespace App\Controller;

use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\Notifier\Notification\Notification;

class SandboxController extends AbstractController
{
    /**
     * @Route("/onboarding", name="onboarding")
     */
     public function __invoke(): Response
     {
       $notification = new Notification('Welcome Sma <3', ['chat/slack']);
       $this->notify($notification);
       // stuff
     }
}

@ismail1432 ismail1432 changed the title add notify helper in abstractcontroller [Notifier] Add notify helper in AbstractController Aug 7, 2021
@chalasr chalasr added this to the 5.4 milestone Aug 7, 2021
@derrabus
Copy link
Member

derrabus commented Aug 7, 2021

Good idea, please add tests. 🙂

@FabienPapet
Copy link

That would be awesome !

@fabpot
Copy link
Member

fabpot commented Aug 7, 2021

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 AbstractController class):

namespace App\Controller;

use Symfony\Component\Notifier\Notification\Notification;
use Symfony\Component\Notifier\NotifierInterface;

class SandboxController
{
    /**
     * @Route("/onboarding", name="onboarding")
     */
     public function __invoke(NotifierInterface $notifier): Response
     {
       $notification = new Notification('Welcome Sma <3', ['chat/slack']);
       $notifier->notify($notification);
       // stuff
     }
}

@OskarStark
Copy link
Contributor

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
Copy link
Member

chalasr commented Aug 7, 2021

The goal of AbstractController is to avoid boilerplate code for common dependencies and tasks, which is what this does.
Even though I don't use it much myself, I think it makes sense as an alternative. Plus, it gives an overview of controllers capabilities which is nice for discovery.
I'm 👍 here, in case we can reconsider.

@OskarStark
Copy link
Contributor

No hard opinion on this 👍🏻

@ismail1432
Copy link
Contributor Author

Thank you for your feedback.

I'm 👎 as we can now easily inject services in controller methods

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.
Having this helper let them kick in Symfony without struggle with these concepts

The cons arguments are relevant but I think this feature is a nice to have and doesn't have bad impact

@wouterj
Copy link
Member

wouterj commented Aug 7, 2021

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 dispatchMessage() (which I honestly didn't know existed, I'm not a huge fan of it either), all methods relate to HTTP world things: building responses, retrieving request info, forms and getting the authenticated user. I don't want to advocate using non-http world things in a controller, as that will encourage building big controllers. For instance, there are also no dispatchEvent() or sendMail() methods.

@derrabus
Copy link
Member

derrabus commented Aug 7, 2021

personally I tend to not use AbstractController anymore, as it makes clear the real dependencies of the class/controller.

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 AbstractController because it makes framework features discoverable.

If developers should avoid using AbstractController, why do we still have that class anyway?

@OskarStark
Copy link
Contributor

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
Copy link
Member

fabpot commented Aug 8, 2021

Thanks for the discussion. I'm now definitely against merging this PR:

  • AbstractController should indeed only be about HTTP related features (everything else should be done in other classes)
  • I don't buy the difficult concept here, we are talking about type-hinting an argument in a method. I don't even think it makes things more or less discoverable as people using the notifier are probably reading some docs before using it.

I've submitted #42422 to take into account our discussion.

@ismail1432
Copy link
Contributor Author

ismail1432 commented Aug 8, 2021

Thanks for the discussion, now the role of the AbstractController is clear for all of us. 👍🏽
I'll close the PR 👌

@ismail1432 ismail1432 closed this Aug 8, 2021
fabpot added a commit that referenced this pull request Aug 9, 2021
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/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.

Commits
-------

f3a6721 Clarify goals of AbstractController
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.

8 participants