-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[WIP] Support for controller autoregistration and autowiring #959
Conversation
I would agree to make Symfony "more friendly" to DunglasActionBundle ... but the following change is a big 👎 from me: 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. |
@javiereguiluz the old controller system still works so is it really a problem ? |
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 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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
I've updated the Symfony demo application to show the benefits of this new system: symfony/demo#315
What do you think @javiereguiluz @wouterj? |
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 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). |
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. |
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 I'm fully open to discuss the directory structure (the bundle can work both with |
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. |
@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. |
According to the Symfony Core Team rules, 👎 votes must be justified by "technical and objective reasons". Here are my reasons to vote 👎:
And now a subjective reason:
We could make 3 things related to this:
This PR goes for 3) My vote is for 1). |
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: 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* @var ContainerInterface
*/
private $container;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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's a very important point. But all examples in doc and training materials will still work.
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.
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.
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
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.
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...
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.
Reducing the number of lines is not always the way to go. But here you actually reduce it (no entry in 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.
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.
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. |
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. |
Closing in favor of #960 |
I've updated the Symfony demo application to show the benefits of this new system: symfony/demo#315
(edited) Features:
@Route
and@Template
annotationsSee symfony/symfony#18193 and symfony/symfony#18193 for the history and the rationale behind this feature.
TODO:
/cc @Ener-Getick