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.All calls to drupal_flush_all_caches() in the installer should no longer be required in the new kernel architecture.
| Comment | File | Size | Author |
|---|---|---|---|
| #26 | install.dfac_.26.patch | 1.89 KB | sun |
| #16 | drupal8.dfac-installer.16.patch | 1.92 KB | sun |
| #11 | drupal8.dfac-installer.11.patch | 1.75 KB | Berdir |
| #6 | no-dfac-with-plugin-manager-clears-2201785-6.patch | 19.66 KB | Berdir |
| drupal8.dfac-installer.0.patch | 1.66 KB | sun | |
Comments
Comment #2
BerdirLooks like all the fails that I looked are because field types are missing?
That's #2155635: Allow plugin managers to opt in to cache clear during module install then, although we could add a workaround and call clearCachedDefinitions() on the field type manager for now, just like we do it for the entity manager.
Comment #3
sunYes, there appear to be two discrete problems:
Routes are not rebuilt (at all).
This causes the interactive installer tests to fail. When redirected to the actual site after installation, you get a 404.
dfac()enforces a rebuild of all routes on every invocation + also callshook_rebuild()in all modules.Technically, #2164367: Rebuild router as few times as possible per request should have resolved this.
But the installer does not actually use a proper
DrupalKernel, soterminate()/shutdown()is never invoked, and AFAIK that's where the solution kicks-in. (?)→ We probably need to add an enforced termination/shutdown to the end of
install_drupal()(which is the legacy, kernel-alike dispatcher that sends the response)Plugin manager caches are never cleared.
Indeed, #2155635: Allow plugin managers to opt in to cache clear during module install could resolve this in a generic way.
In light of a non-interactive/programmed/batched installation (single request), I wonder whether that issue should actually shoot for a solution that is essentially the same as #2164367: Rebuild router as few times as possible per request — i.e., only add a marker/pointer that a rebuild is needed, but don't rebuild immediately?
Or is that irrelevant, because they are using a cache?
But why are they using a cache to begin with? Why are plugin definitions not state?
In any case, in light of this total test suite time on one of our slowest/oldest testbots:
…I'd totally shoot for a temporary stop-gap to clear a few necessary plugin manager caches manually in
ModuleHandler::install(), unless we're able to resolve #2155635: Allow plugin managers to opt in to cache clear during module install in a timely fashion.Comment #4
sunThe spin-off #2202477: Make FieldInfo implement a new CachedStateInterface might allow us to resolve these problems on a universal scale.
Comment #5
Berdir#2155635: Allow plugin managers to opt in to cache clear during module install is blocked on two things:
1) Making it green. Thew new patch might be or at least brings us one step closer.
2) Decide on the exact approach to call the services/methods.
A quick fix here for the field type would require the same fixes as we have in the other issue, at least those that are related to field types (most are). So if that issue gets blocked further on 2), we could move forward here with a direct call.
As commented in the other issues, FieldInfo isn't the problem.
I disagree with persistent cache = state. Plugin caches can and need to be rebuilt when the thing they're caching is changing . State isn't rebuilt, it's just changed explicitly. State must be persistent, caches not because they can be rebuilt.
And yes, plugin cache clearing is already the same as rebuildIfNeeded(). We don't rebuild anything, we just clear the cache, so that whoever is going to get the definitions will trigger a rebuild. If nobody does, then it will never be rebuilt. The only problem, as the other issue showed is that cache clearing isn't a no-op, we could end up doing repeated cache clears even if nobody requested the information in the meantime. But we might be able to optimize that too.
Comment #6
BerdirHere's a combination of the patch here and the now green patch from #2155635: Allow plugin managers to opt in to cache clear during module install.
Only tried the first failing patch from the previous patch and that works now.
Based on that, we should be left with only the router related fails?
Comment #8
sun#2155635: Allow plugin managers to opt in to cache clear during module install is RTBC now.
#2213357: Use a proper kernel in early installer laid the ground for having a proper kernel in the installer at all times.
I'm not sure where exactly the router rebuilding kicks in, but it ought to be possible to manually trigger a rebuild in
install_finished(); i.e., the very last install task/step — so that we only build the router once after installation completed.Comment #9
sundrupal8.dfac-installer.0.patch queued for re-testing.
Comment #11
BerdirRe-roll.
Comment #13
Berdir11: drupal8.dfac-installer.11.patch queued for re-testing.
Comment #15
sunWe still need to implement a manual router rebuild in
install_finished(). Should be easy?Comment #16
sunThis will come back green.
Comment #17
BerdirNice. Yeah, I just did the re-test because I wanted to see the test result on a fast bot.
Looks like it's not as much faster as before, probably because the tests are much faster already anyway and doing less extension scans and router rebuilds isn't that much of a time saver.
It's not really the problem of this issue, but I was wondering if we should do a rebuildIfNeeded() after the module was installed instead of the manual rebuild at the end.
We had the problem that we did something in hook_modules_installed() that resulted in a router lookup for one of the new modules, which didn't work as we no rebuild happened. We can do this ourself, but it would be nice if core could handle that correctly. On the other side, this will probably result in quite a few additional rebuilds as the batch stuff will request routes on every page load?
Comment #18
sunSpent some to tinker about that question...
I actually don't think that any module install hook should rely on possibly existing router information... that sounds like asking for trouble.
The installer itself doesn't actually use any of the module routes. Even when we will have converted it into a micro-app, it's going to use a small set of hard-coded routes.
Lastly, I didn't actually check/confirm yet whether this patch improves the installer performance for me locally...
Comment #19
BerdirHm, Note that I'm talking about hook_modules_installed(), which happens *after* the module installer completed it's job. Shouldn't a module be completely installed at that point, *including* routes?
This is my use case:
I'm using the default_content module from https://github.com/larowlan/default_content, which allows to install default content that from hal+json encoded files. The path alias is a field now, so it is possible to put the alias directly into that file and it works perfectly - except that by the time it now runs, the routes aren't rebuild and the route the alias is supposed to point to does not exist, resulting in an exception. I'm not interested in *accessing* any routes, but they do need to exist in the system to be able to save things that reference them.
There's nobody else that's there to do this, content_translation could but should not care about routes and path.module certainly shouldn't have to worry about this.
You can use entity types and plugins of any type now in this hook (before the plugin cache clearer, we've run into the problem that the rest plugin manager wasn't cleared, so denormalizing them didn't work for entity types that were installed after rest.module), why not routes?
Again, not saying that we have to solve it here, but I feel pretty strongly that we have to solve it somehow and instead of adding a forced rebuild in the installer, we could let the module system deal with it?
Comment #20
sunYeah, just to clarify, I'm not opposed to (re)building the router after installing each module. Retaining that but skipping a full
dfac()should still present a major performance increase, since plenty of unrelated caches are no longer flushed.But I'm confused:
Wasn't the new router supposed to be optimized in the sense of being able to build/install the routes of a particular module only? (instead of rebuilding the whole system)
Why does
ModuleHandler::install()not build the routes of the module being installed? — This appears to be the actual root cause and bug revealed by this patch...?Comment #21
Berdir1. Yes, but I'm not sure if that's just an implementation detail and this point or an actual API. Looking at RouteBuilder, it seems to be the former, it collects all routes and then only the processing/dumping is by provider.
2. Yes, that's kind of what I'm saying :)
Comment #22
tstoeckler#21.1 is correct but even that part is being ripped out as that causes all kinds of problems. See #2158571: Routes added in RouteSubscribers cannot be altered
It would be cool though if we can optimize this, instead of a full rebuild each time.
Comment #23
sunI finally get it now!
ModulesListConfirmForm::submitForm() manually calls
drupal_flush_all_caches()! :-(Comment #24
Berdir16: drupal8.dfac-installer.16.patch queued for re-testing.
Comment #26
sunComment #27
BerdirYeah, let's do this, opened #2248297: Ensure routes are rebuilt when install modules for the route rebuild problem, wasn't caused by this issue in any way, so that shouldn't block it.
As mentioned above, the difference isn't as huge anymore as it used to be, but it's still 27 instead of 31 minutes, based on the few test results that I checked.
Comment #28
catchNice! Committed/pushed to 8.x, thanks!