ControllerBase::container() and FormBase::container() needs to be private

Yes, please.

This has already caused confusion, and there's a language feature that makes it perfectly clear. Let's use it.

Attempt at a comment:

This method is marked private to prevent sub-classes from retrieving services from the container through it. Instead \Drupal\Core\DependencyInjection\ContainerInjectionInterface should be used for injecting services.

Mark asked me to weigh in here. My stance on privates is currently: #2081945-11: [Policy, no patch] Reintroduce 'private' visibility to object-oriented code..

If we do reintroduce privates, then #5 seems a logical place to do it. Although I will note that Symfony's Controller base class has a protected container() method. :-) I think our explanation for doing it differently is that we're trying to educate people about injection, and controllers/forms are the biggest touch-point so it makes more people learn what good injection is.

It's not just that, it's that we want to provide a base class that offers some common services, without making every controller/form container-aware.

This issue came up after a discussion on IRC where there was confusion around whether to inject additional dependencies or to just use the container method that's there. This tells me that we need to clear things up by just making container private.

+++ b/core/modules/locale/lib/Drupal/locale/Controller/LocaleController.php
@@ -51,8 +51,8 @@ public function checkTranslation() {
-      'filter' => drupal_get_form(TranslateFilterForm::create($this->container())),
-      'form' => drupal_get_form(TranslateEditForm::create($this->container())),
+      'filter' => drupal_get_form(TranslateFilterForm::create(\Drupal::getContainer())),
+      'form' => drupal_get_form(TranslateEditForm::create(\Drupal::getContainer())),

See #2112711: Provide an easier mechanism for using drupal_get_form() directly for a way to clean this up.

This is a major issue. Providing the container does not help anyone learn injection, and was not the intention of these classes.

This is best paired with #2110501: ControllerBase should implement ContainerInjectionInterface like FormBase

Discussed on the phone with @Dries, @webchick, @msonnabaum, @tim.plunkett, and @alexpott -- this should be considered a beta blocker given the DX impact.

Let's keep the patch as simple as possible.

So child classes have to use injection but the base class doesn't, and just calls to \Drupal. This is getting nuts IMO.

So child classes have to use injection but the base class doesn't, and just calls to \Drupal. People will just never write tests for this stuff. This is getting nuts IMO.

In general controllers should avoid containing logic from my beginning.

So child classes have to use injection but the base class doesn't, and just calls to \Drupal. People will just never write tests for this stuff. This is getting nuts IMO.

Well, this patch at least does not make the situation worse.
When I would have to write my code in contrib I would probably inject everything properly, though things like ->url()/t() aren't really worth to be unit tested. We should maybe provide helper methods on the base unit test, if possible.

Discussed on the phone with @Dries, @webchick, @msonnabaum, @tim.plunkett, and @alexpott -- this should be considered a beta blocker given the DX impact.

Well, it certainly does not "improve" the DX, though forcing people to do things better is a good thing.

I think this is fine as it is.

Discussed with @catch; it'd technically be okay to put this in after the beta. (Doesn't affect that this issue is RTBC anyway; just major and beta blocker don't make sense together.) :)

I agree with Damian here. Don't particularly object to this, but I think people will just call \Drupal if they're doing it wrong anyway, so leaving for someone else.

I think this makes sense. It makes it so that dependency injection happens consistently in all "user-space" code.

Committed and pushed to 8.x. Thanks!

This was already covered in the change notice at https://drupal.org/node/2168147.

Looks like there is a leftover $this->container() call in ConfigFormBase?

I am getting a

PHP Fatal error: Call to private method Drupal\Core\Form\FormBase::container() from context 'Drupal\Core\Form\ConfigFormBase' in /[...]/core/lib/Drupal/Core/Form/ConfigFormBase.php on line 79

EDIT

Created #2168799: ConfigFormBase fails when invoking $this->container().