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.
At the moment in Simpletest in order to access services and the like we do the following:
$config = $this->container->get('config.factory')->get('whatever');
$config->set('key', $value)->save();
This has several downsides:
- $config is not a typed so you have no idea what interfaces it implements and IDEs can't help
- We are not taking advantage of being able to manipulate the container in tests and in we do we still have update the container in \Drupal just incase
- $this->container is not always maintained leading to issues (@berdir to provide nid)
- it's unnecessarily verbose
I propose the we either
1. use \Drupal class
$config = \Drupal::config('config.factory')->get('whatever');
$config->set('key', $value)->save();
2. implement helper methods.
$config = $this->configFactory()->get('whatever');
$config->set('key', $value)->save();
3. Use the methods automatically created on the container
$config = $this->container->getConfig_FactoryService()->get('whatever');
$config->set('key', $value)->save();
I prefer the option 1 because we do a good job of keeping the container that's injected into \Drupal up to date - and it'll be used all over any remaining procedural code.
Comments
Comment #1
Berdir+1 on option 1, as suggested in my duplicate issue: #2089337: Decide if tests are allowed to use \Drupal::something() instead of $this->container->get().
#1786490: Add caching to the state system is the issue you are looking for, it isn't about that problem, but I encountered it there and had to use \Drupal.
Comment #2
tim.plunkett#3 sounds like #2084637: Use a service container with automated wrappers, which would be cool as hell, but I'm not sure if that will happen.
#2 would nicely match ControllerBase/FormBase, but you'd have to check to see if that method even existed, and fallback to something else when it didn't.
#1 rubs me the wrong way, but it probably is better than $this->container
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedSo long as the container that's in \Drupal is the correct one for tests, option 1 makes the most sense to me. I'm not clear on why it rubs Tim the wrong way; maybe I'm missing something?
Comment #4
tim.plunkettOh, just that we're trying to limit Drupal:: to legacy/procedural code, and I know $this->container is available to me.
So barring #2084637: Use a service container with automated wrappers happening overnight, lets just go with Drupal::
Comment #4.0
tim.plunkettUpdated issue summary.
Comment #5
tim.plunkettIf we're going to do this, we should make this a policy soon.
Comment #6
sunI wasn't aware of this issue/proposal...
I strongly disagree with (1), because it means that we'd be moving in the utterly wrong strategical and architectural direction:
While we have
\Drupal
(and will possibly even release with that), the one and only reason for introducing\Drupal
was to not be blocked on legacy procedural code and thus be able to make progress with OO conversions. Our ultimate goal still is to remove global state constructs, and the\Drupal
global state construct is one part of that.Additionally, #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead will help to resolve a range of inconsistency problems that currently exist for web tests, because due to the lack of a clean test environment separation in HEAD, we are intentionally enforcing a different container between the test runner vs. the test site child environment. That is certainly the topmost reason for why
$this->container
is troublesome in tests right now. → Instead of enforcing a rebuild of a separate container,$this->rebuildContainer()
will essentially just simply (re)load the current container that has been compiled+dumped in the child site already.In short, disagree with the problem statement and proposed solution (1).
(2) and (3) do not change the situation in any way, because it is irrelevant whether you call
$this->container->get('foo')
or\Drupal::service('foo')
— the test runner cannot magically assume that the container of its own process execution environment equals the container of the process execution environment of the child site under test, because they are separate PHP processes. The only way to ensure that the container is the same is to actually load/import the container of the child site.From a technical perspective, only the IDE/type-hinting issue remains valid for me, but that is not limited to the container usage in tests at all; the identical issue exists for
\Drupal::service()
as well as$this->container
in all classes into which the full service container is currently injected.To resolve the general type-hinting issue, we should continue to work on #2084637: Use a service container with automated wrappers
Comment #7
sunI still disagree with this proposal.
Nevertheless, clarifying the issue title to avoid confusion. The whole point is about web tests only.
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedi disagree as well, and completely agree with sun
Comment #9
sunClosely related: #2252967: rebuildContainer() is not limited to web tests (WebTestBase)
Comment #10
mgiffordComment #11
Mile23Did anyone decide this?
Comment #12
Mile23+ a zillion on #6. The reason you have DI is so you can use it in this way. The reason we have
\Drupal
is so that we can migrate away from procedural code. SimpleTest swaps out the container in\Drupal
as a stopgap until procedural code is gone.If you need a strongly-typed service object because you're dealing with it a lot in your test and you want to help the IDE, you should adopt this pattern:
This decision is important because it's blocking a few issues which convert procedural code to OOP, such as this one: #2452577: Remove Usage of deprecated function taxonomy_*
Comment #13
joelpittetCan we close this issue and make a decision here to use #2066993: Use \Drupal consistently in tests Am I completely wrong? Tell me why, just bumping to help unstick this.
The rational @sun made seems to make sense and since it's tests we can still do this in RC.
Comment #14
Mile23$this->container
+1.It means we can more easily switch to a fixture container later if we need to.
Comment #15
Mile23Ugh. Meant to close it out. Argue in the other issue if you disagree please. :-)
Comment #16
tim.plunkettRecategorizing as a duplicate, not a won't fix.