Skip to content
This repository was archived by the owner on Nov 27, 2020. It is now read-only.

[WIP] Support for controller autoregistration and autowiring #959

Closed
wants to merge 5 commits into from

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Mar 20, 2016

  • Integrates DunglasActionBundle
  • Automatically register "actions" as services and enable autowiring

I've updated the Symfony demo application to show the benefits of this new system: symfony/demo#315

(edited) Features:

  • In most cases, this is not necessary anymore to declare services (they are automatically constructed by the autowiring system - DX, RAD)
  • Dependencies are explicit (DX, testability)
  • The container is not injected in controllers anymore
  • It's possible (but not mandatory) to create framework agnostic controllers (especially when using the PSR-7 Bridge)
  • It's possible and recommended (but not mandatory) to use invokable actions (it's not what I've done in my PR on the demo to explicit the upgrade path)
  • It's 100% BC with the traditional controller system, 0 app will be broken, and people preferring the old way can still use it
  • It's 100% compatible with @Route and @Template annotations

See symfony/symfony#18193 and symfony/symfony#18193 for the history and the rationale behind this feature.

TODO:

/cc @Ener-Getick

@javiereguiluz
Copy link
Member

I would agree to make Symfony "more friendly" to DunglasActionBundle ... but the following change is a big 👎 from me:

controller_action

The change may seem "small", but it's a huge change in the way Symfony worked until today. It would require us to make immense changes, such as the documentation and the training materials.

@GuilhemN
Copy link
Contributor

@javiereguiluz the old controller system still works so is it really a problem ?
To satisfy everyone maybe we could include both possibilities in the standard edition: a controller and an action class? WDYT ?

@dunglas
Copy link
Member Author

dunglas commented Mar 20, 2016

Both changes are optional but: it doesn't prevent controllers that have been written following "the traditional way" to work.

You can keep using the Controller directory (but controllers will not be registered as services) and using __invoke (even when controllers are registered as services) is not mandatory (but should be considered a good practice IMO). It's 100% BC.

As the traditional way of writing controllers is still working well, doc can be updated progressively like it has been done for the new directory structure.

@@ -7,7 +7,7 @@ $lineSize = 70;
$symfonyRequirements = new SymfonyRequirements();
$iniPath = $symfonyRequirements->getPhpIniConfigPath();

echo_title('Symfony2 Requirements Checker');
echo_title('Symfony Requirements Checker');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be part of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is automatically updated when running composer update

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure but as this file is always overwritten I think it doesn't hurt.


private $container;

public function __construct(ContainerInterface $container)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed during the hackday, it would be better to inject the parameter directly but it requires to add some lines in services.yml. IMO it's very rare that a controller should use a parameter (parameters must usually be injected in standard services). Maybe the best for here is to remove the use of this parameter?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/**
 * @param ContainerInterface $container
 */
public function __construct(ContainerInterface $container)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, not valid according to Symfony CS (and IMO useless).

@dunglas
Copy link
Member Author

dunglas commented Mar 20, 2016

I've updated the Symfony demo application to show the benefits of this new system: symfony/demo#315

  • In most cases, this is not necessary anymore to declare services (they are automatically constructed by the autowiring system - DX, RAD)
  • Dependencies are explicit (DX, testability)
  • The container is not injected in controllers anymore
  • It's possible (but not mandatory) to create framework agnostic controllers (especially when using the PSR-7 Bridge)
  • It's possible and recommended (but not mandatory) to use invokable actions (it's not what I've done in my PR on the demo to explicit the upgrade path)
  • It's 100% BC with the traditional controller system, 0 app will be broken, and people preferring the old way can still use it
  • It's 100% compatible with @Route and @Template annotations

What do you think @javiereguiluz @wouterj?

@wouterj
Copy link
Member

wouterj commented Mar 20, 2016

Let's say this first: I like the idea behind the action style Symfony way. I think it's a nice way to make decoupling from frameworks and moving to some nicer DDD apps easier (framework agnostic controller will never be possible though imo, but that's another topic).

However, I'm not sure if it should be the default or recommended way. Besides the advantages, it also has some disadvantages. One I can think of now: I have to create one class and one file per controller action. Reducing the amount of files one has to create before having a functional working app was one of the main goals of the best practice guide (which is about the only reason @Route annotations are recommended).

I think while action-style means easier decoupling, using normal controllers is still the RAD-est, easiest way to write your app and I think Symfony Demo and Standard should stick to controller-way for this reason. Also, decoupling is needed in about 10% of the apps, for 90% it will only make developers life more difficult. At last, I believe that controllers shouldn't be unit tested, as controller shouldn't contain logic so there isn't much to test in that unit.

(however, I explicitly voted 😕 instead of 👎 because I'm not very sure about my opinion yet).

@theofidry
Copy link

Reducing the amount of files one has to create before having a functional working app was one of the main goals of the best practice guide

I'm having trouble to understand here: isn't the best practice guide supposed to give guidelines on best practices? Reducing the amount of files looks like DX for me, it's not about best practices, whereas splitting a controller, at least, promotes SOLID principles (how many principles are we breaking with a single controller?!).

I can perfectly understand the reluctance of making the "Action way" the default in Symfony (although I would love to), but it should at least be promoted as a best practice IMO.

@dunglas
Copy link
Member Author

dunglas commented Mar 20, 2016

One I can think of now: I have to create one class and one file per controller action.

No, you can use both action-style controllers and have several action methods per class. Take a look at the PR I've opened on the demo: symfony/demo#315

In term of RAD, not having to declare services in the services.yml file is a big win isn't it?

I'm fully open to discuss the directory structure (the bundle can work both with Controller or Action by default, and both with __invoke and multi-method controllers). I propose the current structure because it (IMO) recommends a clean way to creates SOLID controllers/action. But what I really want to get merged is autowired controller as a service by default.

@wouterj
Copy link
Member

wouterj commented Mar 20, 2016

In term of RAD, not having to declare services in the services.yml file is a big win isn't it?

The first 17 chapters of the Symfony Book are able to create controllers, forms, entities, tests, ... without even introducing the term "service". There is no need to declare services in Symfony using traditional controllers.

@dunglas
Copy link
Member Author

dunglas commented Mar 20, 2016

@wouterj you must introduce a service every time you create business logic. And any small real app has business logic. Services are at the core of the Symfony-way of building apps (and it's basically the main change between Symfony 1 and 2).

A lot of Symfony apps have large controllers with a ton of actions and all the business logic in controllers. I hope this proposal will encourage Symfony developers to keep their controllers as small as possible and to introduce services more frequently (we cannot force people to write good code, but we can encourage them to do it). And btw this PR is vaguely inspired by the ADR pattern but has nothing to do directly with DDD.

@javiereguiluz
Copy link
Member

According to the Symfony Core Team rules, 👎 votes must be justified by "technical and objective reasons". Here are my reasons to vote 👎:

  • It deprecates most of the symfony.com documentation and blog posts written about Symfony until today. This will cause confusion in lots of users. It also deprecates most of the existing training materials.
  • The default controller/action showed to developers imply that you must create 1 file/class per action, which increases "perceived complexity" a lot. (However, you can define several actions per file).
  • I've always heard that PHP's magic methods (like __invoke()) are slow from a performance point of view (I don't have stats to back this affirmation! Could someone shed light on this?)
  • For newcomers, the new way to work feels "unnatural". Believe or not, most developers have never used a trait and consider "__invoke()" a PHP "rarity".
  • It depends on a bundle which hasn't been battle-tested: DunglasActionBundle has 120 stars on GitHub and just 92 installs according to Packagist. I'm sure the code of this bundle is good enough because Kévin is a great developer and knows Symfony well ... but integrating this into Symfony will set a worrying precedent.

And now a subjective reason:

  • If we adopt such a big change, it must be at least 10 times better than the current situation: the developer must be 10 times more productive or the application 10 times more performant, etc.
  • But this change just proposes to use "traits" instead of "extends" and "__invoke()" instead of "indexAction()". Where is the benefit in productivity or performance or DX? In the past you made $this->get('slugger') and now you must do use ...\Slugger and create a constructor to get that service. You don't reduce the number of lines to write or the number of concepts to learn.
  • The main arguments in favor of this change are: 1) you don't have to declare services and 2) controllers are turned into services automatically. However:
    • In real applications you probably need to create those services after all because they are used in other services, in console commands, etc. not only on controllers.
    • Most people don't define their controllers as services. I can't see the benefit of it.

We could make 3 things related to this:

  1. Make Symfony capable of using DunglasActionBundle
  2. Integrate DunglasActionBundle in Symfony itself
  3. Integrate DunglasActionBundle and make it the default way to use Symfony

This PR goes for 3) My vote is for 1).

@Pierstoval
Copy link
Contributor

I'm not a pro in terms of patterns and all these things, I just comment as a regular Symfony user, and I cannot agree more to what @javiereguiluz said:
Making Symfony capable of using Actions system is clearly enough to allow more flexibility and fit to other patterns.

But with my 3-years experience in Symfony 2, I can say that I have never declared controllers as services, and almost always extended Symfony's controller in my own controllers (I can remember only one or two cases, used only for maximal performances).

Refactoring the whole documentation to fit a new design pattern does not sound DX at all to me, because the whole web knows about Symfony's Kernel>Controller>Response system to be a standard and of most common use.

Plus, regarding the recent changes in Symfony's releases schedules, I don't see any benefit to integrate such complex default behavior and make it BC in the next minor releases. The new major releases will come soon enough to avoid having "dirty BC" in most versions, and even if it's personal, I'm afraid that this new design pattern standard would replace Symfony's one, and I don't want to see classic Controllers disappear in a Symfony 5 release for example (ok, it's a bit paranoid, sorry about that).

My "fears" may or may not be reasonable, this is just my comment.

But for all the reasons @javiereguiluz invoked (no ADR pun intended), I'm also 👎 for this 😕

{
use ControllerTrait;

private $container;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/**
 * @var ContainerInterface
 */
private $container;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not useful, IDE can guess it with the constructor typehint.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincentchalamon 👍 , PHP doc above a property allows do not scan the whole file to find what type is it. The question here is not that IDE can do it for you but improve code readability for other developers. BTW, I suppose not everyone IDE can guess it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've no opinion about that but it's not what we do in symfony/symfony and it's not compliant with our current CS. Let's discuss this in another issue.

@dunglas
Copy link
Member Author

dunglas commented Mar 21, 2016

@javiereguiluz OK got most of your points, I reply below and I've proposed an alternative with emphasis on what I mean is the important part for now: #960

It deprecates most of the symfony.com documentation and blog posts written about Symfony until today. This will cause confusion in lots of users. It also deprecates most of the existing training materials.

It's a very important point. But all examples in doc and training materials will still work.
IMO the legacy must not prevent us of moving forward. We need BC, we need an upgrade path (and we have them here), but we also need to always improve Symfony.

The default controller/action showed to developers imply that you must create 1 file/class per action, which increases "perceived complexity" a lot. (However, you can define several actions per file).

I don't agree with you on that point. One of the most common problem in Symfony apps is developers putting all the business logic in controllers. On many large projects, controllers are far too big "god objects" and are a nightmare to maintain.
My proposal promote 3 best practices:

  • extract business logic into services
  • keep your controllers as small as possible (easier to maintain and to understand for newcomers in the project)
  • choose carefully your dependency and don't create god objects

But I can understand that some people like you think it's a too big step for now, it's why I opened #960, but IMO it's better to make both "old" controllers and "new" actions cohabit and promote the new one.

I've always heard that PHP's magic methods (like __invoke()) are slow from a performance point of view (I don't have stats to back this affirmation! Could someone shed light on this?)

You got a point, it should be benchmarked. I think it's slower but it's negligible for the number of actions called in a request. Btw, renaming __invoke to invoke or action will still work but I find that less elegant.

For newcomers, the new way to work feels "unnatural". Believe or not, most developers have never used a trait and consider "__invoke()" a PHP "rarity".

Sure, but it's also the mission of Symfony to educate developers to new PHP features and best practices. And the old system still work.

It depends on a bundle which hasn't been battle-tested: DunglasActionBundle has 120 stars on GitHub and just 92 installs according to Packagist. I'm sure the code of this bundle is good enough because Kévin is a great developer and knows Symfony well ... but integrating this into Symfony will set a worrying precedent.

You're 100% right. It's exactly why I want this merged before the feature freeze: to battle-test it. 120 stars in only 2 months is not so bad IMO (it's a sign of interest for this kind of approach by the community). IncenteevParameterHandler has only 260 stars and has been available for years.

But I'm open to any solution to ease the maintenance: add the SF core teams to the maintainers, move it under the symfony namespace, merge it in core...

If we adopt such a big change, it must be at least 10 times better than the current situation: the developer must be 10 times more productive or the application 10 times more performant, etc.

It's not a so big change. Actually, doing that with Symfony worked for years and the bundle is very small. As the old system will still work, even if it's 10% better IMO we should move forward.

But this change just proposes to use "traits" instead of "extends" and "__invoke()" instead of "indexAction()". Where's the benefit in productivity or performance or DX? In the past you made $this->get('slugger') and now you must do use ...\Slugger and create a constructor to get that service. You don't reduce the number of lines to write or the number of concepts to learn.

Reducing the number of lines is not always the way to go. But here you actually reduce it (no entry in services.yml. It also reduces the number of concept to learn at first (no config file to know with it's own directory structure...). It becomes similar to how Laravel works but with cleaner concepts and extensibility behind.

I've used my bundle on some projects since 2 months and automatic dependency building is very pleasant and time-saver.

Using composition instead of inheritance in such case is also considered universally a best practice.

Easing the migration to framework agnostic controllers is also something that matter in term of code quality and marketing.

In real applications you probably need to create those services after all because they are used in other services, in console commands, etc. not only on controllers.

The bundle supports commands too thanks to @Ener-Getick (see my updated PR on the demo), other services have their their dependency autowired too. It's the main point about this PR. As controllers and commands are the entrypoint of the service graph, in 70 to 80% of cases (maybe more), you don't need to declare your services explictly anymore!

With my proposal, controllers are indeed services but it's 100% transparent for the end user. The intention is not to promote controllers as services, but to use them to unlock new features and improve code quality.

@Pierstoval

Plus, regarding the recent changes in Symfony's releases schedules, I don't see any benefit to integrate such complex default behavior and make it BC in the next minor releases. The new major releases will come soon enough to avoid having "dirty BC" in most versions, and even if it's personal, I'm afraid that this new design pattern standard would replace Symfony's one, and I don't want to see classic Controllers disappear in a Symfony 5 release for example (ok, it's a bit paranoid, sorry about that).

It's not dirty at all. And I'll be 👎 to remove the traditional controller system in any future Symfony release. It can be useful in a lot of contexts.
Technically both my bundle and the "old" system share the same Symfony internals. I've used the traditional controller system for years and I can't find any argument to remove it. The maintenance will be as complex with or without it.
IMO their is no reason to be afraid of this new proposal replacing the traditional way. Both can cohabit without any problem.

@weaverryan
Copy link
Member

Thanks for your work @dunglas. I definitely dislike this - at least as an initially step. But I am interested in your #960. Whatever we do, it will need to be a way to do things, but not the recommended way to do things initially. We have to give ourselves some time using this stuff to see how it feels before we could roll it out as any type of a recommended practice. Personally, I'm very interested, but not convinced of the whole idea - just because we haven't had enough people coding with it long enough.

@dunglas
Copy link
Member Author

dunglas commented Mar 25, 2016

Closing in favor of #960

@dunglas dunglas closed this Mar 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants