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.
So it looks like the majority of our remaining failures are related to the language system. That's not surprising given that the locale system is not hooked into the new path handling at all. I suspect (hope?) that if we can get that wired up it will wipe out a whole bunch of failing tests. Getting that wired in, though, is non-trivial. I am unclear at the moment how much of it is core and how much of it nominally has to go through hooks. It's possible that we will implement a temporary listener that stubs out to a hook, just to make it work, but that will not be the final solution.
Comment | File | Size | Author |
---|---|---|---|
#16 | kernel-bootstrap-cleanup-16.patch | 4.93 KB | effulgentsia |
#12 | kernel-bootstrap-cleanup.patch | 10.15 KB | effulgentsia |
#11 | kernel-bootstrap-cleanup.patch | 9.43 KB | effulgentsia |
#7 | kernel-bootstrap-language.patch | 10.35 KB | sun |
Comments
Comment #1
Jody LynnLanguage negotiation using path prefixes is failing (if you set a second language in locale and try to go to e.g. /fr/admin/reports you get a page not found)
function language_from_url changed $_GET['q'] to remove the language prefix. I tried doing $request->attributes->set('system_path', $path) there, but it gets run before onKernelRequestPathResolve in PathSubscriber which also sets the system_path.
Crell wants to mutate the path and make a new request object.
Comment #2
Crell CreditAttribution: Crell commentedUpdating title.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedHere's the branch commit: http://drupalcode.org/sandbox/Crell/1260830.git/commitdiff/d45fbee?hp=1b....
Test result: #1486960-78: Testing testbot.
Comment #4
Crell CreditAttribution: Crell commentedYowza!
I fixed up the missing documentation in this branch and did a little other cleanup, then merged it into the kernel branch. I'll post a new combined patch for testing shortly.
Thanks, effulgentsia! It looks ugly, but at least it's a contained ugly and documented how to make it unugly. :-)
Comment #5
aspilicious CreditAttribution: aspilicious commentedAre we sure we can move update_fix before full bootstrap?
Comment #6
cosmicdreams CreditAttribution: cosmicdreams commentedDepends. Currently it's not really doing anything. I'm not exactly sure what the intention of that code is but currently it makes no difference. Maybe it would in the future?
Comment #7
sunComment #8
Crell CreditAttribution: Crell commentedsun, I already merged the previous patch so if you have a follow-up please push it as a new branch. Thanks.
Comment #9
Crell CreditAttribution: Crell commentedComment #10
sun_CODE makes sense for the first part, but less so for the latter.
I think we should try to move the unicode and fix_gpc_magic stuff out of _CODE and _FULL entirely into an earlier phase, possibly a new one.
I've no idea why that happens so late in the first place, since there are no dependencies on other stuff.
Would it make sense to create a separate core issue for that, or do you want to adjust it here?
Stale static.
Note that this initialization is currently also performed for other front controllers, except of update.php. I'd recommend to retain that behavior (might even fix some more test failures).
The original code comment clearly states that it's not possible to perform a full bootstrap, prior to performing essential D8 changes.
I'm not sure what the new _CODE bootstrap phase is going to perform in total, but if the validation/correction of input data would be in a cleanly separated phase, then we'd bootstrap to that phase.
(In fact, I'm not even sure how it is possible to bootstrap into _LANGUAGE in a D7->D8 upgrade, since the entire subsystem has changed.)
The call into file_get_stream_wrappers() and possibly also the module_load_all() looks like it could break very easily.
This could use a comment and pointer to LegacyRequestSubscriber, so people don't have to hunt that down on their own.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedSubmitting this as a patch (against kernel branch) rather than a branch commit in case it needs more Dreditor review. This addresses #10 except:
All front controllers except index.php continue to drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL) so that code runs. I think we'll keep that for the initial core patch. In later follow-ups, we'll need to discuss how we want to structure the other front controllers.
Is that code comment correct though? All I see update_fix_d8_requirements() doing is setting a variable that nothing uses. If it's just a placeholder for future update functions, perhaps we should deal with it then, since maybe other bootstrap cleanup will take place before then?
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedActually, this fixes update.php per #10 as well.
Comment #13
sunupdate_fix_d8_requirements() may not contain larger changes now, but will almost certainly later in the cycle. None of the recent Drupal core versions was able to perform upgrades without it.
The adjustment in #12 looks acceptable though.
Note: I've created #1569456: Move replace fix_gpc_magic() with a hard requirement for magic quotes to be disabled for this.
Comment #14
aspilicious CreditAttribution: aspilicious commentedStuff that does weird when adding a new language:
1) Frontpage doesn't load
2) Bartik gets enabled as admin theme ==> WTF??
3) shortcuts dissapears
Does this means we're lacking test coverage?
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedRe #14: #1569680: Fix LocalePathFunctionalTest and system_custom_theme().
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedThis is a reroll of #12 to include only the feedback from #10 not being covered by #1569456: Move replace fix_gpc_magic() with a hard requirement for magic quotes to be disabled.
Comment #17
Crell CreditAttribution: Crell commentedCommitted #16. The rest of the cleanup seems to have been moved to the core queue anyway so let's deal with it there. Thanks!
Comment #18.0
(not verified) CreditAttribution: commentedUpdate description to be a global issue for language-related stuff.