Skip to content

[FrameworkBundle] Introduce AbstractController, replacing ControllerTrait #22157

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

Merged
merged 1 commit into from
Mar 25, 2017

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Mar 25, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no (master only)
Deprecations? yes
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Basically reverts and replaces #18193.

Instead of using getter injection to provide our controller helpers, let's leverage the new ServiceSubscriberInterface (see #21708).
This is what the proposed AbstractController class provides.

So, instead of extending Controller, this would encourage extending AbstractController.
This provides almost the same experience, but makes the container private, thus not usable by userland (this safeguard was already provided by ControllerTrait).

I did not deprecate Controller, but I think we should. Now that we also have "controller.service_arguments" (see #21771), we have everything in place to encourage not using the container in controllers directly anymore.

My target in doing so is removing getter injection, which won't have any use case in core anymore.

The wiring for this could be:

services:
    _instanceof:
        Symfony\Bundle\FrameworkBundle\Controller\AbstractController:
            calls: [ [ setContainer, [ '@container' ] ] ]
            tags: [ container.service_subscriber ]

But this is optional, because we don't really need to inject a scoped service locator: injecting the real container is fine also, since everything is private. And this is done automatically on ControllerResolver.

'security.csrf.token_manager' => '?'.CsrfTokenManagerInterface::class,
);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It's nice that Controller and AbstractController are sharing code (ControllerTrait), but there may be some advantages to duplicating the functions in each. Specifically, error messages. Here are some situations with AbstractController where we should give a really clear error:

  1. I don't understand things, and extend AbstractController on a non-service controller -> setContainer() won't be called (and currently, we'll get a "method called on a non-object" error)
  2. I don't have the _instanceof for AbstractController or I'm missing the calls part. Same error as above.
  3. All of the dependencies in AbstractController are marked as optional. If any of these are truly optional, then the methods that use these services should check if they exist, and throw a clear exception if those methods are called, but the service is missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comments addressed: injecting the real container is fine also, since everything is private. This is done automatically in ControllerResolver if no container was previously set.
About point 3., that's already the case for most service lookups, and not really required for the other services, now that the real container is injected automatically on demand.

Copy link
Member

Choose a reason for hiding this comment

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

I like the DX you created by doing that! But I wish it weren't done in such a one-off way :) (the extra code in ControllerResolver with the whole $previous container thing). We could implement ContainerAwareInterface and just not set the container the second time? Both are a little strange...

@javiereguiluz
Copy link
Member

My comment is not directed specifically to @nicolas-grekas but to all the people that are trying to push ideas like these ones:

  • This provides almost the same experience, but makes the container private.
  • I think we should deprecate the base Controller class.
  • Encourage not using the container in controllers directly anymore.

Are you telling me that putting this line of code in my controllers will turn my application into something horrible and unmaintainable?

$this->get('app.user_manager')->save($user);

I believe that's a great line of code. I love that Symfony controllers provide me a simple access to the container services so I can make my controllers ultra-thin and then I can leverage services in other parts of the application.

Please, don't complicate such a critical feature of the framework. Thanks!

@weaverryan
Copy link
Member

I like this! And the fact that we'll get a lot of great stuff, but no longer need getter injection is awesome 👍

Btw, we should not deprecate Controller. It's just too soon - we need to see how this stuff works in the wild.

@javiereguiluz I think your comment is really important! If this new paradigm is going to work (and we can't rush it), then the user-land code needs to stay as simple as it was before. The "new way" recommendation would look like this:

public function editAction(UserManager $userManager)
{
    // ...
    $userManager->save($user);
}

(thanks to #21771). And as long as we don't deprecate the old Controller class too soon, I think we're good (and we can continue to make sure the DX on the new stuff is as good, and hopefully better).

@javiereguiluz
Copy link
Member

In my opinion, there would be some important DX differences:

Before:

  1. Create a UserManager class
  2. Define a app.user_manager service for that class
  3. Whenever you want to get the user manager, get/inject app.user_manager service

After:

  1. Create a UserManager class
  2. Define a app.user_manager service for that class
  3. If you want to inject the user manager in a service, inject app.user_manager service
  4. If you want to get the user manager in the controller:
    4.1 Open the services configuration, look up the app.user_manager service and look at its associated class (UserManager)
    4.2 Import the UserManager class in the controller
    4.3 Type-hint the controller action with that class (that variable will "magically" contain the service, not the class instance)
    4.4 If more than one service use that class, the above won't work and you need to do other things.

@nicolas-grekas
Copy link
Member Author

100% agree with @weaverryan
@javiereguiluz please don't hijack the PR ;)

@sstok
Copy link
Contributor

sstok commented Mar 25, 2017

Using the Controller class to easily load services using the container is great for newcomers and RAD, for those who prefer more explicit loading (while still being lazy) can use this way or #21771 👍

@nicolas-grekas nicolas-grekas force-pushed the abstract-controller branch 2 times, most recently from 422f073 to 8a395aa Compare March 25, 2017 16:57
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

One minor comment... which I'm not sure can be done in a better way. But, 👍

'security.csrf.token_manager' => '?'.CsrfTokenManagerInterface::class,
);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I like the DX you created by doing that! But I wish it weren't done in such a one-off way :) (the extra code in ControllerResolver with the whole $previous container thing). We could implement ContainerAwareInterface and just not set the container the second time? Both are a little strange...

@nicolas-grekas
Copy link
Member Author

@weaverryan the method is internal so we don't care. And it's better than implementing any interface, which would mean adding public behavior, which should remain private.

@fabpot
Copy link
Member

fabpot commented Mar 25, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit a93f059 into symfony:master Mar 25, 2017
fabpot added a commit that referenced this pull request Mar 25, 2017
…ing ControllerTrait (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[FrameworkBundle] Introduce AbstractController, replacing ControllerTrait

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no (master only)
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Basically reverts and replaces #18193.

Instead of using getter injection to provide our controller helpers, let's leverage the new `ServiceSubscriberInterface` (see #21708).
This is what the proposed `AbstractController` class provides.

So, instead of extending `Controller`, this would encourage extending `AbstractController`.
This provides almost the same experience, but makes the container private, thus not usable by userland (this safeguard was already provided by `ControllerTrait`).

I did not deprecate `Controller`, but I think we should. Now that we also have "controller.service_arguments" (see #21771), we have everything in place to encourage *not* using the container in controllers directly anymore.

My target in doing so is removing getter injection, which won't have any use case in core anymore.

The wiring for this could be:

```yaml
services:
    _instanceof:
        Symfony\Bundle\FrameworkBundle\Controller\AbstractController:
            calls: [ [ setContainer, [ '@container' ] ] ]
            tags: [ container.service_subscriber ]
````

But this is optional, because we don't really need to inject a scoped service locator: injecting the real container is fine also, since everything is private. And this is done automatically on ControllerResolver.

Commits
-------

a93f059 [FrameworkBundle] Introduce AbstractController, replacing ControllerTrait
fabpot added a commit that referenced this pull request Mar 25, 2017
…las-grekas)" (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

Revert "feature #20973 [DI] Add getter injection (nicolas-grekas)"

This reverts commit 2183f98, reversing
changes made to b465634.

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no (master only)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Let's remove getter injection, we now have enough alternative mechanisms to achieve almost the same results (e.g. `ServiceSubscriberInterface`, see #21708)., and I'm tired being called by names because of it.

The only use case in core is `ControllerTrait`, but this should be gone if #22157 is merged.

Commits
-------

23fa3a0 Revert "feature #20973 [DI] Add getter injection (nicolas-grekas)"
@nicolas-grekas nicolas-grekas deleted the abstract-controller branch March 25, 2017 19:54
@fabpot fabpot mentioned this pull request May 1, 2017
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.

6 participants