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.
Investigating this issue, I discovered a very insidious bug in the module installer. It is exposed, under certain circumstances, by the content_translation module -- but by no means is that the only module potentially affected.
Given:
- You don't have Language or Content Translation installed
- You do have a module installed which provides at least one plugin manager which depends, directly or indirectly, on the url_generator service
If you attempt to install Content Translation (allowing the module system to install the Language module implicitly, since it is a dependency of Content Translation), you will get a RouteNotFoundException: Route "entity.configurable_language.collection" does not exist.
What's happening here is this:
- Through the plugin.cache_clearer service, the module installer instantiates all cacheable plugin managers (which is virtually all of them) and clears their caches. This means that your url_generator-using plugin manager is also being instantiated. The url_generator has been fully initialized as one of its dependencies, and it has a reference to normal, non-lazy route provider.
- Just before calling hook_install on the newly installed module(s), the module installer swaps in the lazy-building route provider in place of the normal one. However, the url_generator consumed by your plugin manager still has the old route provider. Because the url_generator is not a factory, that url_generator is the same URL generator as what you'd get from \Drupal::urlGenerator(). In other words, by this point, the global url_generator has a stale route provider, even though the module installer swapped the correct one into the global container.
- Content Translation tries to generate a link to a route that was just created as part of the same install cycle, but because the URL generator does not have the lazy route provider swapped in by the module installer, it does not find the new route, and fails with an exception.
Comment | File | Size | Author |
---|---|---|---|
#25 | 2913912-25.patch | 7.16 KB | alexpott |
#25 | 22-25-interdiff.txt | 1.04 KB | alexpott |
#22 | 2913912-22.patch | 7.15 KB | alexpott |
#22 | 20-22-interdiff.txt | 1.1 KB | alexpott |
#20 | 2913912-20.patch | 7.03 KB | alexpott |
Comments
Comment #2
phenaproximaI attach a failing test for your consideration!
Comment #4
alexpottHere's a potential fix - just swap out the route provider immediately after rebuilding the kernel and before the plugin cache clear.
It also fixes the comment that reference uninstall in the install hook.
Comment #5
dawehnerNice and simple fix!
Comment #7
phenaproximaMaybe I should have added
hidden: true
to the test module?Comment #8
alexpottThis will fix the broken test. @phenaproxima test modules need to be in the Testing package.
I think we need to move the test out of content_translation - because, whilst that module exposes the issue, any module using Url::fromRoute() in hook_install() where there are plugin managers that depend on the route provider will cause this.
Comment #9
dawehner+1, one less test which is bound to implementation detail.
Comment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNice fix! Here's a small review :)
Do we need to make the same change in
\Drupal\Core\Extension\ModuleInstaller::uninstall()
as well?We need to move the test module outside of content_translation as well. And the name of the module is a bit confusing.. it sounds like it provides some routes but instead it just provides a plugin manager that uses the
url_generator
service..I take it that kernel tests do not use the module installer.. somehow?
After the patch is committed, "currently" will lose its meaning, so I think this comment needs to be rewritten a bit :)
Doesn't this mean that when content_translation decides to not use the route provider in its
hook_install()
for whatever reason, we won't be really testing anything?Edit: Nevermind points 2 and 5, @alexpott said the same thing in #8.
Comment #11
alexpottI tried to move the test into
\Drupal\KernelTests\Core\Extension\ModuleInstallerTest()
but this failed because it is KernelTest and there's magic in there that swaps out the route provider for a lazy building one - see\Drupal\simpletest\TestServiceProvider
. Funky stuff.Comment #12
alexpottI considered #10.1 whilst making the change. What is interesting about uninstall is that we never swap
router.route_provider.old
but to berouter.route_provider
back again. I'm really not 100% certain about where we should swap it. Also the swapping feels a bit off because it happens in a possible loop. AH I SEE.. This is actually all okay because the kernel is being rebuilt and so the route provider becomes the correct one each time the kernel is rebuilt. There definitely should be more comments here :) but tldr; I think the uninstall loop is fine - I'm not even really convinced of why it is necessary to swap out for the lazy builder in the uninstall loop (apart from being a mirror image to install).Comment #13
alexpottI've moved all the test coverage outside of the content_translation module and addressed #10. I've added commentary to the ModuleInstaller instead of the tests since I think this is where it might be the most useful.
No interdiff because of all the moves.
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe name of the module is still confusing since it doesn't really provide any routes :)
Leftover content_translation stuff :)
Comment #16
alexpottthanks for the review @amateescu.
1. Well it is the route provider service during install that is causing the issues. I've renamed it lazy_route_provider_install_test because that is specifically what we are interested in. That the lazy route provider is being used.
2. Fixed.
Comment #17
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOk then, ship it! :D
Comment #18
alexpottNoticed a missing "r".
Comment #19
dawehnerI love this piece of documentation!
Comment #20
alexpott@dawehner suggested the test could do with some assertions. Decided to assert on the results of the URL getting.
Also this is now blocking #2571235: [regression] Roles should depend on objects that are building the granted permissions so this issue is borderline critical.
Comment #21
dawehnerI predict that this fails on a subdir installation
Comment #22
alexpott@dawehner can predict the future.
Comment #24
dawehnerYou could use
assertStringEndsWith
, couldn't you?Comment #25
alexpott@dawehner yep that looks simpler.
Comment #26
alexpottGiven this is blocking #2571235: [regression] Roles should depend on objects that are building the granted permissions (which is critical) this is at least a major.
Comment #27
dawehnerThe module installer is a sad place, just so much random hardcoded stuff. Its not something we can change quickly, but at least we document things a bit better with this issue, thank you!
Comment #29
catchFixed this on commit, appropriate for halloween though:
Committed 391b710 and pushed to 8.5.x. Thanks!
Leaving RTBC against 8.4.x for cherry-pick once 8.4.1 is out.
Comment #30
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@catch, don't forget the reviewers, they might be celebrating Halloween too :P
Comment #31
catchSorry got distracted by the local fix, which I then managed not to commit properly. Just reverted in 8.5.x, adding credit, when the 8.4.x freeze is over I'll re-commit to both, leaving at 8.4.x since it's valid for that branch.
Comment #33
phenaproximaKicking back to RTBC for cherry-pick to 8.4.x, and re-queued the patch for testing against 8.4.x. Obviously any further test runs against 8.5.x will fail, since the patch landed there.
Comment #35
phenaproximaNope.
Comment #39
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x, thanks!