Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
drupal_container() is deprecated, and all calls in the system module need to be replaced with Drupal::service(), except for where the module_handler service is requested, which needs to be replaced with Drupal::moduleHandler() (see #1957154: Replace calls to drupal_container()->get('module_handler') service with Drupal::moduleHandler())
This task a part of #2001206: Replace drupal_container() with Drupal::service()
Comments
Comment #1
benjy CreditAttribution: benjy commentedInitial patch to get things started. Pretty much a find and replace however I changed Drupal::service('request') for Drupal::request()
Comment #3
undertext CreditAttribution: undertext commentedUse statements added.
Comment #4
undertext CreditAttribution: undertext commentedComment #5
BerdirDon't use Drupal, use \Drupal. Same for those below.
Comment #6
undertext CreditAttribution: undertext commentedOk. As I understand from https://drupal.org/node/1353118
Comment #7
kgoel CreditAttribution: kgoel commentedIt looks like there are a lot of calls in test files that need to be converted to this->container->get().
Comment #8
jlbellidore-roll
Comment #9
jlbellidoComment #10
jlbellidoI was looking for more instances of drupal_container()->get instances and i didn't find one (i applied #8 patch before search). This is the instances list of drupal_container that i found in system module:
Comment #11
ddrozdik CreditAttribution: ddrozdik commentedneeds reroll.
Comment #12
jlbellidoRerolled #8
Comment #13
jlbellidoSorry, attached .patch
Comment #14
alvar0hurtad0Rerolled and tested (I hope)
Comment #16
alvar0hurtad0Re-trying upload the patch now following the steps at https://drupal.org/patch/reroll
Comment #17
RobLoachShould this be there?
Comment #18
alvar0hurtad0Where?
Comment #19
benjy CreditAttribution: benjy commentedThis line
Comment #20
alvar0hurtad0Ups,
You're right, Sorry and thank you very much :)
Trying again.
Comment #22
alvar0hurtad0Ok,
I know it's an easy task but it's my first patch.
Trying again
Comment #24
benjy CreditAttribution: benjy commented@alvar0hurtad0 not to worry everyone is here to help.
Do you have a another patch applied in this? It seems there is a new test added and some other stuff?
Comment #25
jlbellidoI have reviewed #22 and i think that this are the mistakes :
- This must be deleted , because in HEAD version it isn't:
- This code :
must be :
Because db_query function hasn't drupal_container function and we must preserve HEAD version.
- This code must be deleted too because this code isn't in HEAD version:
- This code must be deleted too for the same reason:
Don't worry,this reroll is very dificult to start. Try it one more time with this changes, i hope that it works.
Regards!
Comment #26
jlbellidoRerolled and run test.
Comment #28
jlbellidoI did #26 reroll past Tuesday , it seems that something has changed again.Trying one more time with a new reroll, this can be applied locally
Comment #29
alvar0hurtad0@Jlbellido you are my heroe.
Comment #30
alvar0hurtad0It applyes ok
Comment #31
jlbellidoComment #32
benjy CreditAttribution: benjy commentedThis should be absolute as per: https://drupal.org/node/1353118
Comment #33
jlbellidoChanged "Drupal::service('path.alias_manager')->getSystemPath()" to "\Drupal::service('path.alias_manager')->getSystemPath()"
Comment #34
penyaskitoReviewed with emphasis on "Drupal" vs "\Drupal" and IMHO looks nice now.
Comment #35
xjm#33: replace-drupal-container-with-drupal-service-2014017-33.patch queued for re-testing.
Comment #37
jlbellidoRerolled #33 and run test.
Comment #39
penyaskito#37: replace-drupal-container-with-drupal-service-2014017-38.patch queued for re-testing.
Comment #40
penyaskitoUnrelated failure, retesting.
Comment #41
penyaskitoBack to RTBC.
Comment #42
ebeyrent CreditAttribution: ebeyrent commentedLooks good to me.
Comment #43
benjy CreditAttribution: benjy commentedAll OO code should have their dependencies injected.
In tests we can use $this->container->get($service_id);
Comment #44
jlbellidoIt needs be rerolled first, atached the rerolled patch again and running tests
Comment #45
jlbellidoChanged "\Drupal::service($service_id)" to "$this->container->get($service_id)" in tests.
However i think that : All OO code should have their dependencies injected. is out of scope.
Comment #46
benjy CreditAttribution: benjy commentedWe don't really want \Drupal::service() calls in our OO code at all because it couples the class to the global Drupal class.
We should either fix the OO stuff here or remove the OO changes (outside of tests) in this patch and create a follow up to do so.
Comment #47
penyaskitoLet's do it here, please :-)
@jlbellido: if you need help ping me on IRC.
Comment #48
andypostno more
out of scope
Comment #49
penyaskitoRerolled pairing phisically with @jlbellido, still needs DI.
Comment #50
penyaskitoTest!
Comment #51
penyaskitoDI was only needed in the already removed class, so this should be ready for review.
Comment #52
penyaskitoClearing tags and tagging with WSCCI.
Comment #53
Crell CreditAttribution: Crell commentedI don't think this is strictly WSCCI, but it's RTBC either way. :-)
Comment #54
catch#49: replace-drupal-container-with-drupal-service-2014017-49.patch queued for re-testing.
Comment #56
jlbellidoRerolled #49 and running tests
Comment #57
dawehnerPerfect
Comment #58
alexpottLets fix this comment. It is not helpful. How about
Comment #59
jlbellidoUpdated the comment in core/modules/system/lib/Drupal/system/Tests/Path/UrlAlterFunctionalTest.php with Alexpott's suggestion.
Thanks Alexpott! :D
Comment #60
Crell CreditAttribution: Crell commentedBack over to you, Alex.
Comment #61
webchickPatch no longer applies.
Comment #62
jlbellidoRelloled #59
Comment #63
dawehnerThis should be \Drupal instead
Comment #64
jlbellidoUpdated with suggested changes of dawehner , thanks!
Comment #66
jlbellidoRetesting
Comment #67
jlbellido#64: replace-drupal-container-with-drupal-service-2014017-64.patch queued for re-testing.
Comment #68
Crell CreditAttribution: Crell commentedAnd back up.
Comment #69
David Hernández CreditAttribution: David Hernández commented#64: replace-drupal-container-with-drupal-service-2014017-64.patch queued for re-testing.
Comment #70
mcrittenden CreditAttribution: mcrittenden commentedTags
Comment #71
LinL CreditAttribution: LinL commented#64: replace-drupal-container-with-drupal-service-2014017-64.patch queued for re-testing.
Comment #73
LinL CreditAttribution: LinL commentedRerolled without the core/modules/system/tests/modules/design_test/design_test.module part as this module has now been deleted from core #2037569: Remove design_test module
Comment #74
Crell CreditAttribution: Crell commentedSigh.
Comment #75
catchI think there's an emerging standard to use \Drupal instead of $this->container in simpletest, but not sure if that's documented yet - at least this makes it easier to find all the $this->container so I went ahead and committed/pushed to 8.x, thanks!
Comment #76
catchI think there's an emerging standard to use \Drupal instead of $this->container in simpletest, but not sure if that's documented yet - at least this makes it easier to find all the $this->container so I went ahead and committed/pushed to 8.x, thanks!