Problem
drupal_container() is deprecated, and all calls in the core modules need to be replaced with Drupal::service().
One exception is where the module_handler service is requested:
drupal_container()->get('module_handler');
which needs to be replaced with Drupal::moduleHandler() (#1957154: Replace calls to drupal_container()->get('module_handler') service with Drupal::moduleHandler())
Proposed resolution
Create patches for each core module.
Remaining tasks
To do
#2001202: Replace drupal_container() with Drupal::service() in the aggregator module
#2001044: Replace drupal_container() with Drupal::service() in the user module
#2003058: Replace drupal_container() with Drupal::service() in the block module
#2003260: Replace drupal_container() with Drupal::service() in the breakpoint module
#2003430: Convert CKEditor tests to use $this->container instead of drupal_container()
#2003498: Replace drupal_container() with Drupal::service() in the comment module
#2003576: Replace drupal_container() with Drupal::service() in the config module
#2003596: Replace drupal_container() with Drupal::service() in the contact module
#2003616: Replace drupal_container() with Drupal::service() in the editor module
#2003630: Replace drupal_container() with Drupal::service() in the entity_reference module
#2011082: Replace drupal_container() with Drupal::service() in the field module
#2011084: Replace drupal_container() with Drupal::service() in the file module
#2011090: Replace drupal_container() with Drupal::service() in the language module
#2011094: Replace drupal_container() with Drupal::service() in the layout module
#2011102: Replace drupal_container() with Drupal::service() in the locale module
#2011104: Replace drupal_container() with Drupal::service() in the menu & menu_link modules
#2011108: Replace drupal_container() with Drupal::service() in the overlay module
#2011116: Replace drupal_container() with Drupal::service() in the path module
#2011122: Replace drupal_container() with injected services in the rest module
#2011126: Replace drupal_container() with Drupal::service() in the search module
#2014009: Replace drupal_container() with Drupal::service() in the serialization module
#2014011: Replace drupal_container() with Drupal::service() in the shortcut module
#2014015: Replace drupal_container() with Drupal::service() in the simpletest module
#2014013: Replace drupal_container() with Drupal::service() in the statistics module
#2014017: Replace drupal_container() with Drupal::service() in the system module
#2014025: Replace drupal_container() with Drupal::service() in the taxonomy module
#2014031: Replace drupal_container() with Drupal::service() in the tour module
#2014033: Replace drupal_container() with Drupal::service() in the content_translation module
#2014037: Replace drupal_container() with Drupal::keyValue() in the update module
#2014043: Replace drupal_container() with Drupal::service() in the views module
#2014049: Replace drupal_container() with Drupal::service() in the views_ui module
...
#2006024: drupal_container() is deprecated, don't use it in bootstrap.php
Comment | File | Size | Author |
---|---|---|---|
#56 | 2001206-container-to-service-54.patch | 13.45 KB | andypost |
#54 | interdiff.txt | 1.83 KB | andypost |
#50 | interdiff.txt | 525 bytes | andypost |
#43 | 2001206-container-to-service-43.patch | 13.42 KB | herom |
Comments
Comment #0.0
ebeyrent CreditAttribution: ebeyrent commentedAdded issue 2003058
Comment #0.1
ebeyrent CreditAttribution: ebeyrent commentedUpdated issue summary.
Comment #0.2
ebeyrent CreditAttribution: ebeyrent commentedAdded issue 2003430
Comment #0.3
ebeyrent CreditAttribution: ebeyrent commentedUpdated issue summary.
Comment #0.4
ebeyrent CreditAttribution: ebeyrent commentedUpdated issue summary.
Comment #0.5
ebeyrent CreditAttribution: ebeyrent commentedUpdated issue summary.
Comment #0.6
ebeyrent CreditAttribution: ebeyrent commentedUpdated issue summary.
Comment #1
ddrozdik CreditAttribution: ddrozdik commentedwe will try to check and create needed patches during Code Sprint UA.
Comment #2
ddrozdik CreditAttribution: ddrozdik commentedebeyrent, 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 ?
Comment #2.0
ddrozdik CreditAttribution: ddrozdik commentedUpdated issue summary.
Comment #2.1
ddrozdik CreditAttribution: ddrozdik commentedAdded new tasks
Comment #3
ddrozdik CreditAttribution: ddrozdik commentedComment #4
ebeyrent CreditAttribution: ebeyrent commentedAbsolutely - please feel free to help out as much as you can!
Comment #4.0
ebeyrent CreditAttribution: ebeyrent commentedadd task for rest module
Comment #4.1
ddrozdik CreditAttribution: ddrozdik commentedadd new tasks to list
Comment #5
ddrozdik CreditAttribution: ddrozdik commentedComment #5.0
ddrozdik CreditAttribution: ddrozdik commentedadd task to list
Comment #5.1
ddrozdik CreditAttribution: ddrozdik commentedadd new tasks to list
Comment #5.2
ddrozdik CreditAttribution: ddrozdik commentedremove duplicated task
Comment #6
Crell CreditAttribution: Crell commentedThe 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*
Comment #7
Ashleigh Thevenet CreditAttribution: Ashleigh Thevenet commentedThe last issue on this meta is now committed, so I'm closing this.
Comment #8
Crell CreditAttribution: Crell commentedUnfortunately 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.
Comment #9
Crell CreditAttribution: Crell commentedLet's try this.
Comment #11
Crell CreditAttribution: Crell commentedComment #12
Crell CreditAttribution: Crell commentedI said needs review you damned buggy issue queue...
Comment #13
tstoecklerSorry, 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.
Comment #14
ebeyrent CreditAttribution: ebeyrent commentedIf we mark the function as deprecated, we're letting the world know that the function is going to be removed at some point. If, however, we're still using it in core, what's the point of it being @deprecated? Core should be the shining example of how to use the API, and to release a version of core that uses deprecated functions is sloppy and lazy.
We should just do the work now and remove the function, which should have been done instead of deprecating the function.
Comment #15
Crell CreditAttribution: Crell commentedActually 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??
Comment #16
tstoecklerWrong 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++
Comment #17
Crell CreditAttribution: Crell commentedCome on, testbot, do your job...
Comment #18
herom CreditAttribution: herom commentedComment #19
herom CreditAttribution: herom commentedComment #20
herom CreditAttribution: herom commentedand back to rtbc, waiting for test results.
Comment #23
herom CreditAttribution: herom commentedreroll
Comment #24
herom CreditAttribution: herom commentedComment #26
penyaskitoFixed typo.
Comment #27
penyaskitoReally weird that this other patch passes.
\Drupal::getContainer() should return the same instance every time is called.
Comment #29
penyaskitoReuploading #26 for showing the difference.
Comment #32
rfayIt 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.
Comment #33
herom CreditAttribution: herom commented29: 2001206-container-to-service-26.patch queued for re-testing.
Comment #36
herom CreditAttribution: herom commented27: 2001206-container-to-service-27.patch queued for re-testing.
Comment #37
herom CreditAttribution: herom commentedpatch #27 is passing.
Comment #38
penyaskitoWhy is the difference in test results between #26 and #27?
This answer must be answered before RTBCing.
Comment #39
herom CreditAttribution: herom commentedwell, 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 newContainer
, and the old one becomes invalid.here is the code path that creates the new
Container
:Comment #40
penyaskitoOk, 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.
Comment #41
Crell CreditAttribution: Crell commented#27 is RTBC. Quick, before someone else uses drupal_container()! :-)
Comment #42
Crell CreditAttribution: Crell commented#27 is RTBC. Quick, before someone else uses drupal_container()! :-)
Comment #43
herom CreditAttribution: herom commentedreroll.
Comment #44
Crell CreditAttribution: Crell commented#43 is RTBC. Quick, before someone else uses drupal_container()! :-)
Comment #45
catchFor 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.
Comment #46
catch43: 2001206-container-to-service-43.patch queued for re-testing.
Comment #47
Dries CreditAttribution: Dries commentedIn one part of the patch, we use:
In another part of the patch, we use:
It looks like further clean-up is required?
Comment #48
catchComment #49
Crell CreditAttribution: Crell commentedRe #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.
Comment #50
andypostRe-roll and fix for #47
Comment #52
catch@Crell I also think we should make an exception in this case, function just will not die.
Comment #53
andypostComment #54
andypostAnd a bit of clean-up of unused variables
Comment #56
andypostComment #57
Crell CreditAttribution: Crell commentedWell, someone beat me to it.
Comment #58
webchickCommitted and pushed to 8.x. Thanks!
Comment #59
ianthomas_ukAnother call was added to core after this patch was committed. I've opened a followup: #2167517: system_custom_theme calls removed function drupal_container