Use \Drupal consistently in tests

The IS mentions both Kernel and Functional tests should use \Drupal::service, but according to alexpott in #2699565-27: Replace \Drupal:: with $this->container->get() in test classes that allow dependency injection of Basic Auth module → this is only true for Functional tests.

I've created an MR with a phpstan rule to identify functional tests that access $this->container. See https://git.drupalcode.org/issue/drupal-2066993/-/jobs/4841841 for results. We could land this and add the existing violations to the baseline. Then we can open follow up issues to cleanup the baseline. I suspect it should be simple enough to write a rector rule to replace $this->container->get() with \Drupal::service().

Removing the original report here as it's just confusing as it was originally the opposite.

IIRC, using \Drupal::service() instead of $this->container->get() addresses issues that surfaced while trying to fix 📌 Memory leak in DrupalKernel when installing modules Active in any test that has a container rebuild. Some of the relevant discussion might've been in Slack, though.

Managed to fix 405 files and only had one fail. Have added a @phpstan-ignore to NodeAccessGrantsCacheContextTest for now. Changing from a Plan to a Task and setting Needs Review.

Some comments.

1. General topic: adding new PHPStan rules to Drupal core. As already found in 📌 Introduce a @return-type-will-be-added annotation to indicate return type addition in next major; enforce with a PHPStan rule Active , new Drupal-defined rules added to Drupal phpstan.neon will not be visible to custom/contrib unless they also add the rule to their projects' neon files. mglaman/phpstan-drupal in that sense is different because in that case all the rules are added as part of the extension installation via composer. Unless we do something, there's high risk that Drupal core own rules will not be checked. The existing rule is different because it only checks Drupal component code that is not present in modules/themes/etc.
2. As discussed in ✨ Add a trait for autowiring properties in tests Needs work , it seems to me that only preventing usage of $this->container->get() is not enough. I understand the problem is that any service should always be accessed from the singleton. Therefore we should also discourage contructs that store a service in a test property or a local variable like
   

     $cache = \Drupal::service('cache');
     ...
     $cache->get(...);
   

   no? That would be tricky.

3. The new PHPStan rule needs tests. Actually see how test coverage instruments in PHPStan rules testing are adding the red line in the MR where the rule is introduced.

Thanks @mondrake, some great points there. My initial thoughts on those:

1. I think automatically applying this rule everywhere in contrib and custom code would be quite disruptive, so it's almost a nice side effect that it would be opt-in. Ultimately I don't really care where it lives. It could probably even be a phpcs sniff that can be auto-fixed with phpcbf.

2. I think that could be handled separately in a follow up? Better to make progress than to get it perfect first go. Unless doing this makes things worse than the current state.

3. Absolutely. Keen to get more feedback first though.

"I understand the problem is that any service should always be accessed from the singleton."

The problem is that the service discovery singleton should actually be a singleton, but Drupal doesn't do that. In theory it shouldn't matter whether we do \Drupal or $this->container, but it does matter, and that's bad.

Since we allow rebuilding the container, and since we allow \Drupal, our code should ensure that any container rebuild ends up in either $this->container, or \Drupal. The test is: does that happen, and is it reflected in our kernel or functional fixture?

Using only \Drupal as a rule means our test will reflect whether all container rebuilds are reflected in \Drupal, which is good. It also means that we never discover whether the test base class' $container property is updated to reflect the not-really-a-singleton, which might matter or might not. Imagine a BTB with $this->container removed, and whether that would be an accurate test of anything. In that world, you're forced to use \Drupal.

Then... In BrowserTestBase, we launch the test into a separate process, which builds the container. In the case of autowiring, as @catch points out in that other issue, you have to be cognizant of which process is doing the autowiring. I think it's better to be explicit about what service lives where, at which point of the test, and so I'd say we should access services local to the test method, and not anywhere else. Basically don't allow \Drupal or $this->container in setup(), but that's really a style thing for me.

So: I'd say if we're going to disallow $this->container, then we should rescope this issue to be to deprecate $this->container. That way there is no confusion about what's going on. If that seems too radical (even though it's essentially what we're doing here anyway), then we should re-evaluate the way Drupal allows rebuilding the service container out-of-bounds.