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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jody Lynn’s picture

Language 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.

Crell’s picture

Title: Fix locale tests » Port language/path handling to the kernel model
Assigned: Unassigned » Crell

Updating title.

effulgentsia’s picture

Status: Active » Needs review
Crell’s picture

Status: Needs review » Fixed

Yowza!

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. :-)

aspilicious’s picture

+// Bootstrap, fix requirements, and set the maintenance theme.
 drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
+update_fix_d8_requirements();

Are we sure we can move update_fix before full bootstrap?

cosmicdreams’s picture

Depends. 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?

sun’s picture

Crell’s picture

Status: Fixed » Needs review

sun, I already merged the previous patch so if you have a follow-up please push it as a new branch. Thanks.

Crell’s picture

Status: Needs review » Needs work
sun’s picture

+++ b/core/includes/bootstrap.inc
@@ -138,12 +138,13 @@ const DRUPAL_BOOTSTRAP_SESSION = 4;
 /**
- * Seventh bootstrap phase: find out language of the page.
+ * Seventh bootstrap phase: load code for subsystems and modules; validate and
+ * fix input data.
  */
-const DRUPAL_BOOTSTRAP_LANGUAGE = 6;
+const DRUPAL_BOOTSTRAP_CODE = 6;

_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?

+++ b/core/includes/common.inc
@@ -5166,7 +5166,7 @@ function drupal_valid_token($token, $value = '', $skip_anonymous = FALSE) {
+function _drupal_bootstrap_code() {
   static $called = FALSE;
 
   if ($called) {

Stale static.

+++ b/core/lib/Drupal/Core/EventSubscriber/LegacyRequestSubscriber.php
@@ -0,0 +1,48 @@
+  public function onKernelRequestLegacy(GetResponseEvent $event) {
+    menu_set_custom_theme();
+    drupal_theme_initialize();
+    module_invoke_all('init');

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).

+++ b/core/update.php
@@ -439,17 +439,9 @@ if (is_null($op) && update_access_allowed()) {
-// update_fix_d8_requirements() needs to run before bootstrapping beyond path.
-// So bootstrap to DRUPAL_BOOTSTRAP_LANGUAGE then include unicode.inc.
-
-drupal_bootstrap(DRUPAL_BOOTSTRAP_LANGUAGE);
-include_once DRUPAL_ROOT . '/core/includes/unicode.inc';
-
-update_fix_d8_requirements();
-
-// Now proceed with a full bootstrap.
-
+// Bootstrap, fix requirements, and set the maintenance theme.
 drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
+update_fix_d8_requirements();
 drupal_maintenance_theme();

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.

+++ b/index.php
@@ -32,7 +32,7 @@ $request = Request::createFromGlobals();
+drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);

This could use a comment and pointer to LegacyRequestSubscriber, so people don't have to hunt that down on their own.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
9.43 KB

Submitting this as a patch (against kernel branch) rather than a branch commit in case it needs more Dreditor review. This addresses #10 except:

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).

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.

The original code comment clearly states that it's not possible to perform a full bootstrap, prior to performing essential D8 changes.

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?

effulgentsia’s picture

Actually, this fixes update.php per #10 as well.

sun’s picture

update_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.

+++ b/core/includes/bootstrap.inc
@@ -138,15 +138,19 @@ const DRUPAL_BOOTSTRAP_SESSION = 4;
 /**
- * Seventh bootstrap phase: load code for subsystems and modules; validate and
- * fix input data.
+ * Seventh bootstrap phase: validate and fix input data.
  */
-const DRUPAL_BOOTSTRAP_CODE = 6;
+const DRUPAL_BOOTSTRAP_INPUT = 6;

Note: I've created #1569456: Move replace fix_gpc_magic() with a hard requirement for magic quotes to be disabled for this.

aspilicious’s picture

Stuff 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?

effulgentsia’s picture

effulgentsia’s picture

This 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.

Crell’s picture

Status: Needs review » Fixed

Committed #16. The rest of the cleanup seems to have been moved to the core queue anyway so let's deal with it there. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Update description to be a global issue for language-related stuff.