Use magic methods to sync container property to \Drupal::getContainer in functional 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/*

The IS mentions both Kernel and Functional tests should use \Drupal::service, but according to alexpott in #2699565-27: Refactor Basic Auth kernel tests to use dependency injection 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 #3492453: Memory leak in DrupalKernel when installing modules in any test that has a container rebuild. Some of the relevant discussion might've been in Slack, though, and the final solution made it not immediately necessary to make the change to use \Drupal::service() instead, but it was thought to be a good idea regardless.

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 #3486376: Extend Symfony DebugClassLoader to report missing cross-module @return types, 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 #3464357: Add a trait for autowiring properties in tests, 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.

Discussed this with @catch because of #3555562: [random test failure] Drupal\Tests\system\Functional\System\ThemeTest::testSwitchDefaultTheme() and while having the discussion I realised that maybe we can use magic methods to enusre we use the container in \Drupal and be done. See https://git.drupalcode.org/project/drupal/-/merge_requests/13675

See https://3v4l.org/hMImt for proof the new MR will work

One question on the MR.

Also, similar to #3544903: [meta] Use property hooks to instantiate services from service closures, this might be something to use property hooks for once on D12.

@godotislate yes! property hooks will work great here. But we can do something now...

Yeah, property hooks would be a follow up once we have a new major branch.

Nah - this ain't going to work in the current form... see https://3v4l.org/6QiDh#v8.4.14

New approach that does work....

added a comment inline

This makes sense as a quickfix for the issues with this, but I still think we should discourage usage of $his->container and deprecate it or something like that. So maybe that should be a child issue of this and this would be a meta/long-term-plan?

It will also fix #3555844: Switching themes in a functionaljavascript test causes backend code to run as anonymous that was reported as a bug/behavior change now that themes rebuild the container. Was always the case for modules.

Going a step further, I've been wondering if we should do something like #3490713: Return the correct typehint when Drupal::service() is called with a class string but specifically limited to test code, so that we get better phpstan/phpstorm (?) detection while not encourage non-injected services outside of test code.

When thinking about #3555562: [random test failure] Drupal\Tests\system\Functional\System\ThemeTest::testSwitchDefaultTheme() before reading this I had a similar idea to the "magic" branch proposed here, nice to see that it works.

This might mean we can remove one or both of these lines in BrowserTestBase::installDrupal():

    // Clear the static cache so that subsequent cache invalidations will work
    // as expected.
    $this->container->get('cache_tags.invalidator')->resetChecksums();

    // Explicitly call register() again on the container registered in \Drupal.
    // @todo This should already be called through
    //   DrupalKernel::prepareLegacyRequest() -> DrupalKernel::boot() but that
    //   appears to be calling a different container.
    $this->container->get('stream_wrapper_manager')->register();

@berdir we can deprecate __get('container') to discourage use and eventually remove it - not sure if we want to do that here or in a followup as it's likely quite disruptive.

Added a comment but I think we could land this as-is and open some followups for deprecations and refactoring?

> @berdir we can deprecate __get('container') to discourage use and eventually remove it - not sure if we want to do that here or in a followup as it's likely quite disruptive.

Yes I was thinking the same thing, doing this would give us a way to properly deprecate it. No strong opinion whether we do this here and have a follow-up for actually deprecating it or if we keep this issue for that. IMHO, that part is/was the scope of this issue, to me it would make more sense to keep this open for that, but no strong feelings. We could also decide to not deprecate it and keep it, but I'm certain that we'll still have discussions in the future about $this->container vs \Drupal::service() if we keep both, because using \Drupal::service() == bad (even though it's now exactly the same)

+1, can we check what would be the impact of deprecating access to $container here before deciding if to do here or in follow up?

Also, I wouldn't have allowed access to dynamic properties at all; minor though as in any case both warnings and deprecations are set to cause tests to fail.

Approx 1k instances of $this->container in Functional/FunctionaJavascript tests... I think that probably needs a meta and childs.

Or we just wait for PHP 8.4 minimum and get and set property hooks like https://3v4l.org/SXiru#v8.4.14 - I think this is the way to go here. I'm not sure of the value of changing everything.

I think given the changes to the theme system in 11.3 and how it has affected tests we should make this an 11.3 release priority.

Did my best to update the title and IS per the new direction in MR 13675.

Needs work for @quietone's comment on the MR, I think we can use the usual format there with deprecated/removed versions.

No wait - what we're doing is restoring a PHP deprecation that the bc layer would otherwise break, so it make sense to have it as close as possible to the message you get from PHP. Restoring RTBC. It will get removed when we can use property hooks.

Actually let's add a bit more of a comment explaining that.

Added comment per #108 and created followup #3558863: Replace magic setter and getter for $container property with property hooks in functional tests. for the property hooks.

That looks great - going to re-RTBC and commit given it's only adding an inline comment.

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!