Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
After enabling modules we do a container rebuild. The very next thing we do is a resetAll which flushes the caches and does container rebuilds.
Comment | File | Size | Author |
---|---|---|---|
#29 | 2189411-29.patch | 4.61 KB | alexpott |
#29 | 24-29-interdiff.txt | 1.21 KB | alexpott |
#24 | 2189411-24.patch | 4.28 KB | alexpott |
#24 | 23-24-interdiff.txt | 869 bytes | alexpott |
#23 | 2189411-23.patch | 3.43 KB | alexpott |
Comments
Comment #1
alexpottLet's see what testbot says.
Comment #2
sunWe should try to get rid of the resetAll() instead. — I don't even understand why that exists in the first place...
Comment #10
dawehnerLet's see what happens if we do this now.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedTriaging issues in simpletest.module as part of the Bug Smash Initiative to determine if they should be in the Simpletest Project or core.
This looks like it a Phpunit issue, changing component.
Comment #14
alexpottComment #15
alexpottThis is definitely still an issue - I was looking at this code a couple of days ago and thought this was silly and also that I'd filed an issue about it before - TIL - 6 years ago.
Comment #16
longwaveThis was improved in #2795789: Do not rebuild the container so often in BTB so not sure this is still relevant?
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedYes, the rebuild of the container was removed in #2795789: Do not rebuild the container so often in BTB.
But there is another code change in this small patch. I can't comment if this is needed or not.
Comment #18
longwave$this->prepareRequestForGenerator();
is already called fromrebuildContainer()
so I think that part of #10 is obsolete as well.There is also an @todo linking to #2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation so I am fairly sure this issue can be closed as duplicate or obsolete.
Comment #19
alexpottThere is a rebuild that we should be able to remove.
Comment #20
alexpottComment #23
alexpott\Drupal\views\Tests\AssertViewsCacheTagsTrait::assertViewsCacheTags() is creating the request so that an unprimed url generator won't be able to create the correct absolute URL. This only works by accident at the minute :(
Comment #24
alexpottI think there's one more container rebuild that we can get rid of. Container's are cached in the database and this is specific to the site.
Comment #25
alexpottHere's a comparison of running the InstallerPerformanceTest with Blackfire instrumentation - https://blackfire.io/profiles/compare/7b6854ab-d5e8-49bf-95ac-f09b2f8bfe...
The change in #24 doesn't do much because we're rebuilding the container after the install anyway. This is because we don't allow the install container to be dumped - see \Drupal\Core\Installer\InstallerKernel::initializeContainer(). But I think it is good leave in because it is incorrect and unnecessary.
So this change results on one less container rebuild and one less router rebuild.
Comment #26
longwaveThe change in InstallerPerformanceTest proves that there is one less rebuild, and the Blackfire results back this up.
Comment #27
alexpottI think we need to file two follow-ups before this can be RTBC.
I think this indicates that we have a subtle installer bug. The system.css_js_query_string should be set during an install but it's not because we don't call drupal_flush_all_caches during the installer - and have not for ages. This has been hidden because we've been calling it in tests - so removing that call exposes the bug.
I think we should have a follow-up to remove this in Drupal 10. Tests should create request's properly - see changes to FrontPageTest here - but let's not break it in this patch.
Comment #28
Berdir1. Could also be related to the memory cache backend we use during install and update, we also had to add a drush cc js+css in our deploy scripts because something is not properly cleared during the cache clear that happens during updates.
Comment #29
alexpottI've created the follow-ups:
@Berdir I don't think this is memory cache related see #3207893: Set system.css_js_query_string during install - would be interested in an issue outlining the problems you are seeing with updates.
Comment #30
longwaveFollowups filed, new @todo comments are helpful, this is ready to go.
Comment #31
larowlanCrediting @longwave for reviews and forensics/triage
Crediting @quietone for forensics/triage
Comment #32
larowlanCommitted db3ad8f and pushed to 9.2.x. Thanks!
I think there is the possibility of disruption to contrib tests here, so erring on the side of caution and leaving it as 9.2 only at this point rather than backporting.
Happy to hear arguments for backporting.