Use \Drupal consistently in tests

A massive patch like this is going to conflict with a lot of the patches being worked on at MWDS. Can we not break it up?

Hm. Wait, this is in the test classes themselves?

I'm going to ask the MWDS sprinters to take care of this as part of their router conversions. I think that's easiest and least conflicting.

added 2067007

This is wrong, its the opposite of #2087751: [policy, no patch] Stop using $this->container in web tests.

if this issue is irrelevant, can we close it ?

I am removing the Novice tag from this issue because:

Avoid issues that are:
...
reopened, derailed, or shifting direction, because they can be very confusing.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

+a lot on this one, - a lot on the other one.

Would be nice to get a decision and close one of these two issues, this can still make it in RC if it's improving tests.

I feel this one is likely the right way to go as well but I'd like someone more involved to help move these forward a bit.

$this->container gives us the option of using a fixture container, whereas we lose the possibility of isolation with \Drupal.

@Mile23 There is no patch, that status doesn't work here. Thanks for making a decision hopefully this doesn't get into a 'big debate' and some agreements can be made.

Anyways this is a meta/plan. Needs child issues and an update grep in the issue summary.

Changed the grep to git grep '\Drupal::' core/modules/*/src/Tests/*.

git grep --name-only '\Drupal::' core/modules/*/src/Tests/* | wc -l tells us there are 709 affected files.

Switching to 8.1.x.

Any final decision can we start with the patch because it will need a huge effort may be some error can be faced.

@snehi maybe try to do as much of a mass find/replace as you can and see how many or few breaks you get from the change. Post the patch here and if they are fixable you can do the remainder of the changes here otherwise start splitting them up by module like the issue summary recommends or by another logical division that shows itself from the test results.

We really should break it out into modules, just so it's easier to review.

@Mile23 if only trivial breaks happen in a find-replace it's much easier to review and commit here. If the scope grows or there are lots of special cases it will naturally need to be split out.

Added a patch for simpletest, which can also serve as a template for other child issues: #2644256: Replace \Drupal:: with $this->container->get() in test classes of simpletest module

I did a few variations of grep to figure out the extent of the issue. git grep -c '\Drupal::' core/modules/*/src/Tests/* | wc -l gave 716 occurrences. Looking at the patches - aggregator and simpletest it appears to me that find and replace isn't going to solve the problem because there is a unique variable involved in each one. But then I could be out to lunch as I'm just starting to look at this.

If we do this piecemeal it is hard to make core consistent. See https://www.drupal.org/core/scope - by module is rarely the best way to split things up... by \Drupal::method might be better...

But also

This will allow for greater latitude in mocking services and filesystems for core.

How exactly does this change allow for that?

"This will allow for greater latitude in mocking services and filesystems for core." How exactly does this change allow for that?

Because \Drupal is an anti-pattern which lets us be lazy about dependencies. Code which uses \Drupal will also use stuff like DRUPAL_ROOT even though there's a service for getting the root directory.

Eventually we want to be rid of \Drupal, which means being rid of .inc files and so forth, and all the rest of the anti-pattern stuff like DRUPAL_ROOT. We can't test for that, however, if we're using \Drupal in any of the tests.

So in order to mock whole filesystems and create fixtures, we have to remove DRUPAL_ROOT and other anti-patterns, and in order to do that we have to inject the container properly.

\Drupal is scaffolding from a dev process we never finished.

So are we still debating whether we should do proper injection in tests? Because we really need to normalize on either $this->container or anti-patterns.

@Mile23 a short, less-defensive answer would be more effective. I figure @alexpott is just trying to get clarification on the "claim" so that as many people can get behind the change without that nagging feeling we are doing busy work with no measurable gains.

Is there maybe an example of something that we are trying to do now that is blocking mocking objects? You've done a lot of work with the tests maybe you've run into an example or two? If there is not, the simplest solution is just remove that statement from the issue summary.

DRUPAL_ROOT removal doesn't really come into play in this issue maybe there is a releted issue for that?

@Mile23 a short, less-defensive answer would be more effective

That's the problem: There's no short answer if you don't already know why this is the better path. Maybe @sun is more credible: #2087751-6: [policy, no patch] Stop using $this->container in web tests

I figure @alexpott is just trying to get clarification on the "claim" so that as many people can get behind the change without that nagging feeling we are doing busy work with no measurable gains.

I thought the same thing.

Here, I'll try it another way: Normalizing on \Drupal is the *wrong* way, because then we'll have the same set of busy work of re-normalizing to a properly-injected container when we finally decide to get rid of global functions or other similar architectural considerations, because it will be the *opposite* of allowing greater latitude in mocking services and filesystems for core.

Is there maybe an example of something that we are trying to do now that is blocking mocking objects?

It's not a question of whether there are current blockers, because it's such an effective anti-pattern. It's a question of do we want to normalize on the injected container that allows us the possibility of testing fixtures? Or do we want to normalize on \Drupal, which ties us to a pattern of globals?

@Mile23 thank you, so the statement can just be removed from the issue summary then I guess?

This will allow for greater latitude in mocking services and filesystems for core.

Because you are saying we 'could' but shouldn't rely on globals.

For @alexpott's other point on how to approach the patch. I noticed we are replacing a bunch of different yet similar methods. Maybe we can pick one in particular and focus on removing that across core entirely?
Like one issue to replace all \Drupal::moduleHandler(), another to replace \Drupal::entityManager() in tests? It would be trivial to review then and maybe easier to get in? Does that work better @alexpott?

How about this change to the summary.

Totally OK with whatever implementation.

I'm pretty sure that $this->container is as much anti-pattern as \Drupal::service(). I agree that doing this now will make it easier to remove \Drupal when all dependencies are injected in run-time code - so that is a good reason to do this.

@joelpittet - yes having an issue per method means that find and replace and scripting is possible - plus very easy to review.

We also should open an issue against coder to add a rule for this.

I'm pretty sure that $this->container is as much anti-pattern as \Drupal::service().

How so?

See #1596472-90: Replace hard coded static cache of entities with cache backends where $this->container broke previously passing tests in combination with another patch.

That's exactly the kind of problem this issue is trying to fix. We should do one or the other, not both. Then we'd know that if $this->container breaks your test then there's something wrong with your expectations. As it is, they could be out of sync anywhere and that's bad.

Say it with me: \Drupal is an anti-pattern. :-)

And once more, please no.

See #2818335: Needs tests: Addthis pubid is incorrectly added . I have to install the modules like this because I need to enable the theme first for the default config to work. However, that means $this->container is now out of sync and points to an old container instance. and since $this->config() uses $this->container, I get weird exceptions about the schema not being defined.

I was able to figure out because I know about problems like this and saw what $this->config() does and worked around it with \Drupal::. No idea how someone else is supposed to figure this out.

I have no issue with *actually* injecting dependencies in any real code. But I don't see a single benefit of using $this->container or $container over \Drupal with the current API in tests

1. We will never test test code, we never need to mock something in test methods. And even in a context where we would, using $container means you gain almost nothing. Mocking it still requires to set up a mock container and set services on it. You save exactly one line, and that is setting the mock/test container for \Drupal.
2. We still depend on the container which means we use it as a service locator and not as dependency injection.
3. We have considerably worse DX when you can't use helper methods like entityQuery(), entityTypeManager() and so on without type hints. You can get that back with plugins but most don't have those installed I think?
4. $this->container in tests is just as global as \Drupal with the main difference that it can be the *wrong* global container ;)
5. We don't want people to inject the container into their services (except for the few cases where that makes sense), so it's not about teaching them how to do things "properly", as @alexpott said, this is just as much an anti-pattern.

Yes, we can work on specific points like improve DX for using $container but that will not change anything about it being an anti-pattern just as much as \Drupal. We could use $this->container() to be able to access the correct global container, but where's the difference to \Drupal then?

To summarize, -1 to this issue.

Let's retitle the issue and reverse the damage already done in existing tests.

See #2818335: Needs tests: Addthis pubid is incorrectly added . I have to install the modules like this because I need to enable the theme first for the default config to work. However, that means $this->container is now out of sync and points to an old container instance.

Yup. That's a bug in the module installer, not the testing system. Use BrowserTestBase::rebuildContainer().

It even has a @todo to the bug #2021959: Refactor module handling responsibilities out of DrupalKernel

That's why we want $this->container, not \Drupal. So we can improve core and get rid of \Drupal.

@Mile23 in that case we should switch to \Drupal consistently here, then fix #2021959: Refactor module handling responsibilities out of DrupalKernel, then a third issue to switch to $this->container if it actually resolves the issue.

@Berdir's comment in #35 pretty much sums up how I feel about this. I really don't think tests have any business interacting with $this->container and in fact I'm really not keen on Web/Functional tests interacting with \Drupal either. This has all been discussed before on #2087751: [policy, no patch] Stop using $this->container in web tests. The problem is that the anti pattern is both cases - using \Drupal or $this->container are equally bad from an architectural point of view - and one ($this->container) turns out to be more buggy.

Re #40: So if you think both are an antipattern, how do you envision tests working without using the container? Or do you think there should just be another way to interact with the container that's neither $this->container nor \Drupal::?

($this->container) turns out to be more buggy.

I'm not sure how.

The point here is that we have to decide to either: 1) Normalize on \Drupal, thus making it OK for the container to go out of sync in our testing expectations, or 2) Keep track of the container within our testing expectations and use $this->container.

But if it's buggy, where's the issue? It would be titled 'Pseudoglobal \Drupal and test fixture $this->container go out of sync sometimes.' And it would end up closed (works as designed) because we have rebuildContainer(). So then we ask: Why should we have to know when to sync the containers? And I'd say yes, that's a good question. Let's fix it so the container reflects installed extensions without re-synching, because the bug is not within the testing system, the bug is discoverable through testing.

If we punt and say, 'Just call \Drupal, it's easier,' then \Drupal is an anti-pattern, because we avoid fixing things that are wrong.

The problem is that the anti pattern is both cases - using \Drupal or $this->container are equally bad from an architectural point of view

This is semi-true, but only because we use both patterns and not one or the other. $this->container gives us more discipline for the test, and allows us to move away from \Drupal. I thought the goal was to adopt IoC and move away from \Drupal, which is why I care about this.

I thought the goal was to adopt IoC and move away from \Drupal, which is why I care about this.

They're both the service locator pattern, the fact that one uses a class property and one a static method on a class does not make it IoC vs. not IoC.

@tstoeckler see the issue I linked in #41

@tstoeckler I don't think many tests should interact with the container tbh. But using $this->container or \Drupal is just the same thing. It is using the container as a service locator - this is the anti-pattern. \Drupal or $this->container is not a move towards inversion of control (IoC) at all.

I can see a use case for getting services from the container in KernelTests but in my opinion this should be done through a method rather than a property. But there really is no case for Web or Functional tests to do it. They should use the UI. If there are things that such tests need to be able to create such as blocks, nodes or whatever not through the UI for the sake of speed well these should be utility services which dependencies injected and when called ensure they are using the current child sites container and not some old container that does not reflect the state of the site.

A test having to call rebuildContainer() is a bug. Because if it is calling that then the container has already been rebuilt on the child site... how many times do we want to rebuild?

@catch:They're both the service locator pattern, the fact that one uses a class property and one a static method on a class does not make it IoC vs. not IoC.

If your code (not the test) uses the IoC pattern, then you can inject mock service into it with ease and it never occurs to you that the test will need to sync \Drupal with anything else, because you're already doing the right thing.

If your code (not the test) locates services through \Drupal, then control has not been inverted, by definition. Then you have to care about whether the containers are synced up, and you have to balk at the idea of BrowserTestBase::rebuildContainer(), and you have to dread having this conversation with Mile23. :-)

Also, you can't prove that you inverted control if you manage \Drupal in your test, other than to clear its container. So while it's true that accessing a service from \Drupal or $this->container in the test doesn't have much to do with IoC, the increased discipline of $this->container allows you to prove that you've inverted control in the code you're testing.

So if the goal is to promote IoC in Drupal and avoid the pseudoglobal, then don't normalize on \Drupal in tests.

@alexpott: I can see a use case for getting services from the container in KernelTests but in my opinion this should be done through a method rather than a property. But there really is no case for Web or Functional tests to do it. They should use the UI. If there are things that such tests need to be able to create such as blocks, nodes or whatever not through the UI for the sake of speed well these should be utility services which dependencies injected and when called ensure they are using the current child sites container and not some old container that does not reflect the state of the site.

+1 on this sentiment. However: The example from #35 is a JavascriptTestBase test, which inherits from BrowserTestBase which has protected $container. Should we change it to private $container? How will we then address the problem @berdir ran into?

@alexpott: A test having to call rebuildContainer() is a bug. Because if it is calling that then the container has already been rebuilt on the child site... how many times do we want to rebuild?

BrowserTestBase::rebuildContainer() is a workaround with a @todo referring to an issue, so yes, it's already bug-ish. The question is: Will we fix the actual bug, or make another workaround for the workaround by normalizing on \Drupal?

This is kinda related http://symfony.com/blog/new-in-symfony-3-3-getter-injection

This is one of a major issue in https://gitlab.com/weitzman/drupal-test-traits. The $this->container property was added because core setup traits can be used easily but if during the test container is rebuilt, e.g. enable/uninstall the module, $this->container now holds the old container so always user `\Drupal::service()`.

Removing the '\Drupal is an anti-pattern' tag because it is an old news. The container pattern is built on the assumption that it will not be rebuilt mid-request but in the Drupal land, we don't follow that rule.

Given #52 reverting the title.

It may be old news, but \Drupal is still an anti-pattern, particularly in functional tests.

Removing the '\Drupal is an anti-pattern' tag because it is an old news. The container pattern is built on the assumption that it will not be rebuilt mid-request but in the Drupal land, we don't follow that rule.

And that's a bug in our assumptions about the container, that we won't fix if we normalize on \Drupal in either tests or code that should have injected services.

Reminder that @Berdir is always right, and we should be using \Drupal not $this->container

OK, so needs a follow-up to deprecate $this->container in three frameworks.

I think we can leave Simpletest test well alone.

• \Drupal\Tests\UnitTestCase - unit testing doesn't have a container.
• \Drupal\KernelTests\KernelTestBase - there is a code that actively keeps the $container and \Drupal's correct - to be honest doing anything here I think it is not worth it.
• BrowserTestBase is where we have the problems with using the container - either via \Drupal or $this->container - doing both is suspect. \Drupal\Core\Test\FunctionalTestSetupTrait is where $this->container is set but the property does not seem to be defined AFAICS. We also have \Drupal\Tests\BrowserTestBase::$originalContainer - see \Drupal\Core\Test\FunctionalTestSetupTrait::getDatabaseTypes() for some horrific fun.

For me the question is what does good practice actually look like for BrowserTestBase? At the moment it is way too easy to write fragile tests that don't use the same container as the site-under-test. We should concentrate on fixing that. Yes using \Drupal makes things slightly easier than $this->container. But there are other bad practices such as doing $this->service = \Drupal::service('foo') in a test set up method. The test purist would say we should go for the Nightwatch approach and not allow any directly API access in BrowserTestBase and it's descendants. I'm incredibly sympathetic to that approach and would love to not bootstrap Drupal to run tests. But doing this will involve another round of re-writing all the tests and they will get significantly slower. So I'm not sure that that is pragmatic. I'm not sure what the pragmatic approach is.

For people still convinced that $this->container is a good idea in BrowserTestBase tests here's further proof of the problems it causes - #3089656-8: Update autoloader prior to including a .module file in ModuleInstaller

More proof of the problem in use $this->container and the whole issue of accessing services in the functional tests... #2160091-48: drupal_rebuild() rebuilds container twice, since drupal_flush_all_caches() also rebuilds it

is there an issue for replacing `Drupal::` in test modules, core/modules/*/tests/modules/ not the tests themselves? It seems we sometimes use it and sometimes not.

Since simpletest is no longer in core moving to the testing component, phpunit.

The title of this contentious issue was updated in #39, #43, and #53. The issue summary was not updated, and it now says the opposite of the title.

I am adding the tag for an IS update.

I hesitate to make this update myself, since I do not know enough to agree or disagree with the decision. But no one else seems interested in updating the issue summary as suggested in #68, so I will do it.

My version of the issue summary refers to kernel and functional tests. I think that is correct. I am sure that referring to SimpleTest does not make sense any more.

Since this ticket is quite long and contradictory I've posted an article with examples to show the issue and use cases https://www.enik.pro/drupal/tests/2022/10/02/service-containers.html, but without solution, sorry.

Regarding core modules:

• Unit tests:
  • Commands: git grep '\Drupal::' */tests/src/Unit/* git grep '$this->container' */tests/src/Unit/*
  • Any usage of \Drupal:: or $this->container outside a setUp method is suspicious.
  • ✅ No suspicious usage detected
• ✅ Kernel tests - not checked as it is not worth it.
• Functional tests:
  • Commands: git grep '\Drupal::' */tests/src/Functional/* git grep '$this->container' */tests/src/Functional/*
  • ❌ Potential suspicious usage detected.
  • To define:
    • Container of which Drupal instance will be used by \Drupal:: calls?
    • Container of which Drupal instance will be used by $this->container calls?
    • Cases when and which method of reaching the container to use for testing and/or testable instance of Drupal setup.
• FunctionalJavascript tests:
  • Commands:git grep '\Drupal::' */tests/src/FunctionalJavascript/* git grep '$this->container' */tests/src/FunctionalJavascript/*
  • ❌ Potential suspicious usage detected.
  • To define:
    • Container of which Drupal instance will be used by \Drupal:: calls?
    • Container of which Drupal instance will be used by $this->container calls?
    • Cases when and which method of reaching the container to use for testing and/or testable instance of Drupal setup.
• ✅ Nightwatch tests are Javascript tests.
• Traits definitions:
  • Commands:git grep '\Drupal::' */tests/src/Traits/* git grep '$this->container' */tests/src/Traits/*
  • ❌ Potential suspicious usage detected.
  • To define:
    • Are those trait methods used in proper contexts?

A similar check needs to be done for core API:

• BuildTests tests:
  • Commands:git grep '\Drupal::' */tests/Drupal/BuildTests/* git grep '$this->container' */tests/Drupal/BuildTests/*
  • ✅ No suspicious usage detected. Only a couple of \Drupal::VERSION calls were detected.
• FunctionalJavascriptTests tests:
  • Commands:git grep '\Drupal::' */tests/Drupal/FunctionalJavascriptTests/* git grep '$this->container' */tests/Drupal/FunctionalJavascriptTests/*
• FunctionalTests tests:
  • Commands:git grep '\Drupal::' */tests/Drupal/FunctionalTests/* git grep '$this->container' */tests/Drupal/FunctionalTests/*
• KernelTests tests:
  • Commands:git grep '\Drupal::' */tests/Drupal/KernelTests/* git grep '$this->container' */tests/Drupal/KernelTests/*
• ✅ Nightwatch tests are Javascript tests.
• Tests tests:
  • Commands:git grep '\Drupal::' */tests/Drupal/Tests/* git grep '$this->container' */tests/Drupal/Tests/*