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.Problem/Motivation
Drupal currently has two main ways of getting the service discovery container:
- \Drupal
- ContainerInjectionInterface or similar 'proper' injection pattern.
\Drupal is intended to be used by legacy procedural code which does not have an injection path.
Other service discovery should follow the inversion-of-control principle where services are injected into consumer classes without keeping a reference to the container itself. This principle is satisfied by the ContainerInjectionInterface pattern, and is also generally satisfied by the arguments property of the service definition file.
Proposed resolution
- #2380293: Properly inject services into ModuleInstaller discovered that some re-thinking is required for the 'lazy' status of some services.
\Drupaldeferred instantiation of some services until needed, and removing it led to large memory usage. So: Discover which services need deferred instantiation as we go along. - Convert controllers/forms which use service traits, such as
StringTranslationTrait. One issue per trait. - Make changes per
\Drupalservice method. There will be some patch conflicts based on which lands first. Since we're injecting the dependencies, we'll probably end up changing the constructor per patch and that will lead to rerolls. We should also look for the service name itself, so we can convert\Drupal::get()for the equivalent service. - We do this for non-test code under this issue.
- Then we can concentrate on making the tests happen: #2066993: Use \Drupal consistently in tests For instance, the majority of occurrences of
\Drupal::root()appear in tests. - #2066993: Use \Drupal consistently in tests suffers a bit from the same reviewability problem as this one, but could be rescoped to work per-method. There would be fewer patch conflicts, because the container is injected into the test class whole.
- Then the plan all goes to heck when we get to
\Drupal::get()and it's utter chaos. :-)
Comments
Comment #2
Mile23Comment #3
Mile23Comment #4
xjmhttps://www.drupal.org/core/scope has some guidelines on how to best scope issues (based on our attempts and mistakes in the past to make codebase-wide changes). Let's brainstorm a bit about how we can make these changes in the most successful way, so we don't lose time needing to redo things across many issues.
I think one patch per controller is going to be ultimately difficult to complete effectively. See https://www.drupal.org/core/scope#consistency for example. Instead, what about grouping issues by the specific type of fix? For example, we could take one method on
\Drupal, replace all its uses in code that should have the dependency injected, and then deprecate the method.It would also be good to document the overall plan and scope here before we start.
Thanks for your ongoing and seemingly tireless efforts to clean up these kinds of technical debt in Drupal 8. On the recent Drupal survey a "fully object-oriented API" was the top thing to improve for developers. I think cleanups like these are exactly what that means.
Comment #5
Mile23Totally agreed that changes have to be consistent and reviewable.
Let's kick it this way:
\Drupalmethod, though there will be some patch conflicts based on which lands first. Since we're injecting the dependencies, we'll have to change the constructor per patch and that will lead to rerolls. We should also look for the service name itself, so we can convert\Drupal::get()for the equivalent service.\Drupal::root()appear in tests.\Drupal::get()and it's utter chaos. :-)There's also a question of metrics, because #2380293: Properly inject services into ModuleInstaller was added and then removed due to memory usage. Do we have a standard way to evaluate this?
Comment #6
Mile23Updated issue summary.
Comment #7
Mile23Added child issue #2776385: Replace non-test usages of \Drupal::transliteration() with IoC injection
Let's see how it goes. :-)
Comment #9
Mile23Child issue: #2801817: Inject services into SimpletestTestForm and functional cleanup for the test_discovery service
Comment #10
xjmThis is listed as the next step for #2704871: Replace usages of deprecated method drupal_render() but I don't actually see an explicit followup for the render usage yet; can we add one? Thanks!
Comment #11
Mile23Voila: #2834982: Replace non-test usages of \Drupal::service('renderer') with IoC injection
Comment #15
Mile23New child: #2945247: Remove usages of @deprecated simpletest functions
Comment #19
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedNew child: #3107003: https://www.drupal.org/project/drupal/issues/3107003
Comment #20
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedNew child: #3106761: \Drupal calls should be avoided in classes, use dependency injection instead in NodeRevisionAccessCheck.php
Comment #21
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedNew child: #3106228: \Drupal calls should be avoided in classes, use dependency injection instead in user module
Comment #22
xjmAs a reminder, we should not "Replace calls to
\Drupalinfoo.module". Instead, we should "Replace calls to\Drupal::bar()in baz usecase." Reference: https://www.drupal.org/core/scope#filesFor example, #2776385: Replace non-test usages of \Drupal::transliteration() with IoC injection has correct issue scope, but #3110333: Reduce \Drupal usage in user module run-time code does not.
Thanks for your efforts to help work on these issues!
Comment #23
xjmAlright, I've closed several issues that were incorrectly scoped as duplicates of the meta:
It's better have issues that look one by one at
\Drupal::foo()usage on a per-method basis, rather than a per-module basis, and fix them core-wide as appropriate. And if the scope of that turns out to be too large, then we should break it down an additional level with (e.g.) "Use$this->foo()instead of\Drupal::foo()in form classes."I definitely recommend novice contributors and their mentors carefully read https://www.drupal.org/core/scope and then use that as a guide for creating and organizing cleanups such as these. That way, you will have more success at getting these cleanups done, and it will be easier for reviewers and committers too.
Note that there is also #3113904: [META] Replace t() calls inside of classes to use injected
$this->t()where available, so be sure not to duplicate that issue either. (Some of those may also have problematic scope by module, but at least it's only one type of change, and there are enough uses oft()that breaking it down some is necessary.)Thanks everyone!
Comment #24
jungletraitscan be used to clean up\Drupal::foo()usages in tests at least. In general, no IOC injection in tests .