Replace drupal_container() with Drupal::service()

we will try to check and create needed patches during Code Sprint UA.

ebeyrent, I see this is not all list of issues, because other modules like field, file, layout ... uses drupal_container function. Do you need help with these modules ?

Updated issue summary.

Added new tasks

add new tasks to list

add task to list

add new tasks to list

remove duplicated task

The issues listed are all done, but it looks like someone has added a bunch more drupal_container() calls. :-( NetBeans finds 34 instances. Let's just fix them here and be done with it.

Do people not read change notices, or do committers not notice when someone is using a deprecated function? *sigh*

The last issue on this meta is now committed, so I'm closing this.

Unfortunately I just checked the source code and not only do we still need to remove the function itself but there's 28 NEW instances of drupal_container(). Someone didn't get the memo. :-(

Let's turn this back into a non-meta to just finish it off. Also tagging beta blocker as there's no good reason for us to ship with the old function.

Let's try this.

I said needs review you damned buggy issue queue...

Sorry, but this is in no way a beta blocker. I'm not a fan of deprecated functions, but nothing at all is or can be broken by the function or its usage. It's not nice and removing it is a nice to have, but to block anything on it is just wrong. Even if we were to ship Drupal 8.0 with the function no one should lose sleep over it.

Actually the fact that drupal_container() can't be autoloaded means that it *can* hurt things. It's been on the chopping block for months, if not a year. Kill it.

If you disagree that it's a beta blocker, review and RTBC the patch sooner rather than later so that it becomes a moot point.

Testbot, do you job, would you??

+++ b/core/lib/Drupal.php
@@ -106,8 +106,8 @@ public static function setContainer(ContainerInterface $container) {
+   * @deprecated This method is only useful for the testing environment. It
+   * should not be used otherwise.

Wrong indentation.

Also: This comment has never been accurate. As can be seen in current_path() but also in many cases in core, there are legit cases for using getContainer(). It always pisses me off when PhpStorm complains at me about using deprecated functions, when there's no alternative...

I still think this issue is a *huge* misuse of the "beta-blocker" tag and a great example for some of the inequalities in the core queue, but it's also very simple and I can't argue it's not RTBC. So marking as such. progress++

Come on, testbot, do your job...

and back to rtbc, waiting for test results.

reroll

Fixed typo.

Really weird that this other patch passes.

\Drupal::getContainer() should return the same instance every time is called.

Reuploading #26 for showing the difference.

It looks to me like the fail of #27 is a result of some difference in behavior of testbot 953. Would love if somebody took a look. Doing a full reconfirmation on it, but my bet is that these are poor tests exposed by a slightly different install procedure.

29: 2001206-container-to-service-26.patch queued for re-testing.

27: 2001206-container-to-service-27.patch queued for re-testing.

patch #27 is passing.

Why is the difference in test results between #26 and #27?
This answer must be answered before RTBCing.

well, to be clear, there is nothing wrong with this patch or the current behaviour.
the interdiff in #27 is necessary, because ModuleHandler::install/uninstall creates a new Container, and the old one becomes invalid.
here is the code path that creates the new Container:

ModuleHandler::install/uninstall() ->
\Drupal::service('kernel')->updateModules() ->
$this->boot() ->
$this->initializeContainer() ->
\Drupal::setContainer($this->container);

Ok, so if this is the desired (and known) behavior, #27 is ready to RTBC.
Thanks @herom for the explanation. While I did the patch myself, I had not a clear vision of the difference.

#27 is RTBC. Quick, before someone else uses drupal_container()! :-)

#27 is RTBC. Quick, before someone else uses drupal_container()! :-)

reroll.

#43 is RTBC. Quick, before someone else uses drupal_container()! :-)

For that reason (patches like this conflcting with new usages, head breaking etc.), I've been asking people to split the actual removal into a separate issue, so the conversions don't get lumped in. Sending for re-test now.

43: 2001206-container-to-service-43.patch queued for re-testing.

In one part of the patch, we use:

-$password_hasher = drupal_container()->get('password');
+$password_hasher = \Drupal::service('password');

In another part of the patch, we use:

-$password_hasher = drupal_container()->get('password');
+$password_hasher = \Drupal::getContainer('password');

It looks like further clean-up is required?

Re #45: Normally I agree, but this issue is mostly just last stragglers and usages that it looks like were added after this issue was setup. That's why we're trying to just close the deal once and for all.

I'll reroll for #47 this weekend if someone doesn't beat me to it.

Re-roll and fix for #47

@Crell I also think we should make an exception in this case, function just will not die.

And a bit of clean-up of unused variables

Well, someone beat me to it.

Committed and pushed to 8.x. Thanks!

Another call was added to core after this patch was committed. I've opened a followup: #2167517: system_custom_theme calls removed function drupal_container