8000 Extending from AbstractController in controller skeletons by yceruto · Pull Request #20 · symfony/maker-bundle · GitHub
[go: up one dir, main page]
Skip to content

Extending from AbstractController in controller skeletons #20

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
Nov 23, 2017
Merged

Extending from AbstractController in controller skeletons #20

merged 1 commit into from
Nov 23, 2017

Conversation

yceruto
Copy link
Member
@yceruto yceruto commented Nov 18, 2017

https://symfony.com/doc/master/best_practices/controllers.html#controllers
[Best Practice] Make your controller extend the AbstractController base controller provided by Symfony and use annotations to configure routing, caching and security whenever possible.

https://symfony.com/doc/master/best_practices/controllers.html#fetching-services
[Best Practice] Don't use $this->get() or $this->container->get() to fetch services from the container. Instead, use dependency injection.

@weaverryan
Copy link
Member

This is a tricky one. I agree with the best practice, but I’m not sure if we should yet push people to AbstractController.

But, if you’re generating a controller, then it’s new code. So maybe? We’ve decided on the docs to keep showing Controller for at least one version more.

@nicolas-grekas
Copy link
Member

I'm personally 👍: now that services are private by default, making them public does require typing configuration.
But injecting services doesn't.
Since we need to teach something to ppl in both cases, better teach them the 3 injection strategies possible for controllers (regular, action, and service subscriber.)

@dunglas
Copy link
Member
dunglas commented Nov 21, 2017 •
edited
Loading

Controller.php.txt doesn't need to extend anything. I would keep it a standalone class.

For ControllerWithTwig.php.txt, I would inject Twig in a constructor or directly in indexAction. Coupling this class to the framework (bundle) just for the render method (that is a proxy method) is not worth it.

Also, I would rename the indexAction method to __invoke. It's more explicit, more PHPish, and it allows to simplify the configuration. See symfony/symfony#24637 for instance.

@dunglas
Copy link
Member
dunglas commented Nov 21, 2017 •
edited
Loading

Also, Sensio\Bundle\FrameworkExtraBundle\Configuration\Route can be replaced by the one provided in the Routing component directly (one less dependency to a bundle).

@nicolas-grekas
Copy link
Member

Controller.php.txt doesn't need to extend anything. I would keep it a standalone class.

I don't agree with this: the helpers provided by the base class are not "coupling" anymore, thanks to the dedicated service locator that is injected. Extending the AbstractController allows better self-discovery of the features provided by Symfony.

@nicolas-grekas
Copy link
Member

I would rename the indexAction method to __invoke

I don't agree with this: this actively prevents putting more actions into one controller. I understand that one-controller-class-per-route is a common practice, yet I don't think it should be the default one.

@dunglas
Copy link
Member
dunglas commented Nov 21, 2017

I don't agree with this: this actively prevents putting more actions into one controller.

It's not the case. You can have both an __invoke method (for the index), and an item() method for instance.

I understand that one-controller-class-per-route is a common practice, yet I don't think it should be the default one.

I definitely prefer the one-controller-class-per-route approach. It should at least be used if the controller contains 1 action (as in the generated one).

@dunglas
Copy link
Member
dunglas commented Nov 21, 2017

the helpers provided by the base class are not "coupling" anymore

You need framework-bundle to use this class.

@yceruto yceruto force-pushed the abstract_controller branch from 902855c to f5aa030 Compare November 21, 2017 13:35
@dunglas
Copy link
Member
dunglas commented Nov 21, 2017

Anyway, it would be nice to let the user configuring the strategy he wants. What about a config flag to choose globally between AbstractController (we should at least use this one instead of Controller) + fooAction (beginner, enabled by default) and extend-nothing + __invoke (confirmed, enabled trough a config flag)? I can work on this.

@yceruto
Copy link
Member Author
yceruto commented Nov 21, 2017 •
edited
Loading

I definitely prefer the one-controller-class-per-route approach. It should at least be used if the controller contains 1 action (as in the generated one).

What about a new controller skeleton for ADR pattern?

namespace App\Controller;

use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Annotation\Route;

/**
 * @Route("{{ route_path }}", name="{{ route_name }}")
 */
class {{ controller_class_name }}
{
    public function __invoke()
    {
        return new Response('Welcome to your new controller!');
    }
}

@dunglas
Copy link
Member
dunglas commented Nov 21, 2017

@yceruto same idea (but strictly speaking it's not the ADR pattern so it should no be called ADR-something - there is no Responder in my proposal).

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Nov 21, 2017 •
edited
Loading

framework-bundle is a mandatory requirement of Flex anyway

and extending this class is not what I would actually call coupling in this case, because the base class provides only simple helpers, vs a full chain reaction of dependencies when Controller is needed (the container)

What about a new controller skeleton for ADR pattern?

let's keep things simple really

@dunglas
Copy link
Member
dunglas commented Nov 21, 2017 •
edited
Loading

framework-bundle is a mandatory requirement of Flex anyway

Yes, but it doesn't mean that the "controller" (action?) class has to be coupled with it too.

extending this class is not what I would actually call coupling

It means that you need this specific lib and its dependencies to execute it.

Anyway this PR is a step in the good direction. Let's merge it, we can discuss this elsewhere.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Nov 21, 2017 •
edited
Loading

and its dependencies to execute it

that's exactly where I stop agreeing :) the base class has no dependencies at all that you don't want/need.

@dunglas
Copy link
Member
dunglas commented Nov 21, 2017

It depends, we reuse some API Platform controllers with Laravel (yes, it works). It will not be possible to do it without requiring framework-bundle (it will work, but it's not very friendly). Anyway it's just an example (not very relevant because it's a lib and not a private project).
The real reason for me to not extend AbstractController is that it feels "magic" (while it's not anymore at all), on the other end using autowiring you inject only, and exactly what you need. It's more explicit.

That being said, I agree that AbstractController allows to discover some features of the framework (IMO it can also be solved by the documentation).

@stof
Copy link
Member
stof commented Nov 22, 2017

Also, Sensio\Bundle\FrameworkExtraBundle\Configuration\Route can be replaced by the one provided in the Routing component directly (one less dependency to a bundle).

That won't work fine. you cannot use routing annotations without FrameworkExtraBundle anyway (we plan to change this for a future 4.x release)

@nicolas-grekas
Copy link
Member

you cannot use routing annotations without FrameworkExtraBundle

what do you have in mind? we made the routing annotation work with services, which was missing before. Is there anything else left?

@stof
Copy link
Member
stof commented Nov 22, 2017 •
edited
Loading

what do you have in mind? we made the routing annotation work with services, which was missing before. Is there anything else left?

The Routing component only provides an abstract AnnotationClassLoader which is a partial implementation. The concrete implementation still lives in the bundle.
FrameworkBundle has its own concrete implementation, but with less features than the FrameworkExtraBundle (for instance, support for services is only in SFEB, not in FB AFAICT, but this is also the case for cleaner __invoke)

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Nov 22, 2017 •
edited
Loading

@stof could you create an issue so we can work on that?

@yceruto thinking a bit more about it, I have another argument for using the annotation from the bundle: it could ease discovering the other possible annotations ("what's in this namespace?"), so OK to revert that change on my side.

@stof
Copy link
Member
stof commented Nov 22, 2017

@nicolas-grekas here it is: symfony/symfony#25103

@yceruto yceruto force-pushed the abstract_controller branch from f5aa030 to b1701e2 Compare November 22, 2017 12:30
@fabpot
Copy link
Member
fabpot commented Nov 23, 2017

Thank you @yceruto.

@fabpot fabpot merged commit b1701e2 into symfony:master Nov 23, 2017
fabpot added a commit that referenced this pull request Nov 23, 2017
@fabpot
feature #20 Extending from AbstractController in controller skeletons…
02aeae2 
… (yceruto)

This PR was merged into the 1.0-dev branch.

Discussion
----------

Extending from AbstractController in controller skeletons

> https://symfony.com/doc/master/best_practices/controllers.html#controllers
> [Best Practice] Make your controller extend the `AbstractController` base controller provided by Symfony and use annotations to configure routing, caching and security whenever possible.

> https://symfony.com/doc/master/best_practices/controllers.html#fetching-services
> [Best Practice] Don't use `$this->get()` or `$this->container->get()` to fetch services from the container. Instead, use dependency injection.

Commits
-------

b1701e2 Extending from AbstractController in controller skeletons
@yceruto yceruto deleted the abstract_controller branch November 23, 2017 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@dunglas dunglas dunglas approved these changes

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yceruto @weaverryan @nicolas-grekas @dunglas @stof @fabpot
0