In BrowserTestBase::installDrupal() we call ::initKernel() to get a reference to the container, but the container has just been built in the installer and is available via Drupal::getContainer(). We should be able to use that rather than building it again, hopefully. If so it will save 1-2s from every browser test.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | Screenshot from 2026-08-10 17-46-19.png | 291.7 KB | catch |
| #14 | Screenshot from 2026-08-10 17-46-28.png | 289.35 KB | catch |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
catchHarder than it looks - also have to set the $kernel property and there's nowhere to get that from, and the kernel would be an InstallerKernel so can't use that anway.
Comment #5
catchMaybe not the original idea but there's another container build that takes 1-2s that I think we can drop.
Comment #7
catchThe stream wrapper unregistration here dates back to when we still had the simpletest UI module in core. Now there's no parent site involved in the PHP process at all.
Comment #8
catchThe new version seems more doable but there are weird errors with the error handler and drupalGet() going to the wrong URL, so will take a bit of effort to get going I think.
Comment #9
longwaveWhat's actually broken here given the MR is green?
Comment #10
catchI hadn't actually pushed the commit that removes the kernel boot itself, just pushed that - functional tests get an error about the error handler not being properly restored and also 403s instead of 200 on the first request (at least that's the result for node module's GenericTest).
The error handler issue should be fixable by moving that code elsewhere, we could try to inline it.
But the 403 it wasn't immediately obvious what's going on.
Comment #11
sjpagan commented@catch tested this locally. The 403 looks like an ordering problem.
bootEnvironment() now runs in the installer, after prepareEnvironment() gave the
prefix to drupal_valid_test_ua(). DRUPAL_TEST_IN_CHILD_SITE is then TRUE in the
test runner, drupal_generate_test_ua() reads a .htkey that was never written and
returns NULL, so the request has no test user agent.
Calling bootEnvironment() before the prefix is set fixes it here. The red Kernel
job is the same 403, in TestSiteApplicationTest.
I tested six commits on your branch: the Installer test group passes locally.
Only the pipeline can confirm it. Want me to push them?
Comment #12
catch@sjpagan it would be good to see the code. If you're personally happy with the changes, they're relatively concise and build on existing work, then pushing to my branch is fine and appreciated.
In cases where there's a completely different approach taken or the change is purely speculative/debugging, opening a new draft branch on a new fork in the same issue to start with can be easier. Then the approaches can be reconciled from there.
Either way I haven't done any active work on this issue since my last comment and have some other issues to tend before getting back to this one, so you won't be interrupting anything.
Comment #13
sjpagan commentedThe pipeline found other errors of the same type, outside the installer group.
Six tests call \Drupal::service('file_system') from prepareEnvironment() or installParameters(), which run before any container exists.
They now call mkdir(), as about other tests already do there.
Comment #14
catchThanks for pushing those changes. Profiled with main vs the MR to confirm we properly drop the container build and we go down from 8 in main to 7 with the MR. Attaching xhgui screenshots for before/after.
There are a couple of things that could use review here:
- we still call some DrupalKernel methods including ::bootEnvironment() even though we don't boot a kernel, I think that's fine.
- we should add a code comment explaining that we're explicitly trying to avoid booting the kernel / building the container so that someone doesn't try to re-introduce the boot to simplify things.
- If we make this major only, and we think that the raw
mkdir()is fine, then that change might be fine with a CR - it's only a few test classes that do it. Or do we want to add some kind of API that does some of what the service API method does statically? Not sure that's necessary given the raw calls to mkdir() work well enough.Comment #15
sjpagan commentedOuch :) on 7 August #3614153 added a
\Drupal::service('file_system')call toDrupalFlushAllCachesInInstallerTest::installParameters(). This branch predates it, so the pipeline is green. The call breaks once the branch is rebased.Can I rebase and fix it before the two review points?
Comment #16
catchI just ran into that trying this change alongside another one, fine to fix that before addressing other review points - helps when the MR is already green to see if futher refactoring regresses something.
Comment #17
sjpagan commented@catch & @longwave all done :)
- Rebased on main. A change from #3614153 added a container call to
DrupalFlushAllCachesInInstallerTest::installParameters();it now uses mkdir(). The pipeline is green.-
ModuleWeight::sort()is now static, and InstallerConfigDirectoryTestBase calls it directly instead of repeating the algorithm. A unit test covers the sorting. That answers part of the static API question: no new API was needed, an existing method dropped its object state.- Six test callers still use
\Drupal::service(ModuleWeight::class)->sort(). That still works, because PHP resolves a static method called on an instance. I left them for a follow-up so this MR stayed green.- One backwards compatibility note: a subclass overriding sort() as an instance method would now fatal. Core has none.
Comment #18
nicxvan commented@sjpagan thanks! Btw the moduleWeight class is brand new so I'd be surprised if someone was already overriding it.
I don't have an objection to it being static, just not really sure why we couldn't just use the service call, is it just too early in the process?
Comment #19
sjpagan commented@nicxvan Yes, too early.
prepareEnvironment()runs with no container, so\Drupal::service()throws ContainerNotInitializedException. That is why the sort was inlined there in the first place.The rebase gave a live example: DrupalFlushAllCachesInInstallerTest hit that exception until its service call became a plain mkdir().
:)
Comment #20
nicxvan commentedThanks! That part looks great to me!