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

Berdir’s picture

+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.

tim.plunkett’s picture

#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

effulgentsia’s picture

So 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?

tim.plunkett’s picture

Oh, 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::

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

If we're going to do this, we should make this a policy soon.

sun’s picture

I 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

sun’s picture

Title: [policy, no patch] Stop using $this->container in Simpletests » [policy, no patch] Stop using $this->container in web tests
Issue summary: View changes

I still disagree with this proposal.

Nevertheless, clarifying the issue title to avoid confusion. The whole point is about web tests only.

ParisLiakos’s picture

i disagree as well, and completely agree with sun

sun’s picture

Mile23’s picture

Did anyone decide this?

Mile23’s picture

+ 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:

class MyTestThatDealsWithTheEntityManager extends TestBase {

  /**
   * Accessor for the entity manager service.
   *
   * @return \Drupal\Core\Entity\EntityManager
   */
  protected function getEntityManager() {
    return $this->container->get('entity.manager');
  }
  
  public function testTheEntityManagerSomehow() {
    $em = $this->getEntityManager();
    $em->autocompleteThisMethodName.....
  }

}

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_*

joelpittet’s picture

Can 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.

Mile23’s picture

Status: Active » Reviewed & tested by the community

$this->container +1.

It means we can more easily switch to a fixture container later if we need to.

Mile23’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

Ugh. Meant to close it out. Argue in the other issue if you disagree please. :-)

tim.plunkett’s picture

Status: Closed (won't fix) » Closed (duplicate)

Recategorizing as a duplicate, not a won't fix.