There are a few places in core that include (actually require) inc files through paths stored into cache/system variables. This can prevent the D8 upgrade from completing successfully, since the path variables don't have the /core segment, hence files cannot be found and included.

I tried to perfom an upgrade with the attached DB (a clean D7 install with Locale and a few language options enabled) and I had to hack the following places to be able to complete the upgrade:

diff --git a/core/includes/bootstrap.inc b/core/includes/bootstrap.inc
index 9780ed2..42ad7a2 100644
--- a/core/includes/bootstrap.inc
+++ b/core/includes/bootstrap.inc
@@ -3056,7 +3056,7 @@ function _registry_check_code($type, $name = NULL) {
   $cache_key = $type[0] . $name;
   if (isset($lookup_cache[$cache_key])) {
     if ($lookup_cache[$cache_key]) {
-      require_once DRUPAL_ROOT . '/' . $lookup_cache[$cache_key];
+      require_once DRUPAL_ROOT . '/core/' . $lookup_cache[$cache_key];
     }
     return (bool) $lookup_cache[$cache_key];
   }
diff --git a/core/includes/common.inc b/core/includes/common.inc
diff --git a/core/includes/gettext.inc b/core/includes/gettext.inc
diff --git a/core/includes/language.inc b/core/includes/language.inc
index 685a4d4..c159ddb 100644
--- a/core/includes/language.inc
+++ b/core/includes/language.inc
@@ -193,7 +193,7 @@ function language_negotiation_get_switch_links($type, $path) {
   foreach ($negotiation as $id => $provider) {
     if (isset($provider['callbacks']['switcher'])) {
       if (isset($provider['file'])) {
-        require_once DRUPAL_ROOT . '/' . $provider['file'];
+        require_once DRUPAL_ROOT . '/core/' . $provider['file'];
       }
 
       $callback = $provider['callbacks']['switcher'];
@@ -339,7 +339,7 @@ function language_provider_invoke($provider_id, $provider = NULL) {
     }
 
     if (isset($provider['file'])) {
-      require_once DRUPAL_ROOT . '/' . $provider['file'];
+      require_once DRUPAL_ROOT . '/core/' . $provider['file'];
     }
 
     // If the language provider has no cache preference or this is satisfied
diff --git a/core/includes/locale.inc b/core/includes/locale.inc
diff --git a/core/modules/locale/locale.install b/core/modules/locale/locale.install
diff --git a/core/modules/locale/locale.module b/core/modules/locale/locale.module
index 18cf99d..60a1e18 100644
--- a/core/modules/locale/locale.module
+++ b/core/modules/locale/locale.module
@@ -1046,7 +1046,7 @@ function locale_url_outbound_alter(&$path, &$options, $original_path) {
 
         foreach ($negotiation as $id => $provider) {
           if (isset($provider['file'])) {
-            require_once DRUPAL_ROOT . '/' . $provider['file'];
+            require_once DRUPAL_ROOT . '/core/' . $provider['file'];
           }
 
           // Avoid duplicate callback entries.
diff --git a/core/modules/locale/locale.test b/core/modules/locale/locale.test

Obviously this is not the right solution, since after the upgrade is complete almost everything is going to break, but it shows where the problems lie. I'm afraid we need to update the variables above before running the upgrade, even worse probably even before finishing to bootstrap update.php. AAMOF the code in the first hunk is executed from _drupal_bootstrap_full(), while the others are involved in the language initialization process.

CommentFileSizeAuthor
#21 update-1331370-21-test.patch200.36 KBplach
#21 update-1331370-21.patch201.81 KBplach
#16 silly.diff.txt885.71 KBAnonymous (not verified)
#13 lol-wut-upgrade.patch201.47 KBAnonymous (not verified)
#11 1331370_11.patch200.36 KBAnonymous (not verified)
#5 lol.wut_.upgrade.patch1.42 KBAnonymous (not verified)
upgrade_path-7_8-core_dir.sql_.zip169.13 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

We do have a running practice of doing one-off functions for large systemic changes like this to run before the general update process. See http://api.drupal.org/api/drupal/includes--update.inc/function/update_pr... and http://api.drupal.org/api/drupal/includes--update.inc/function/update_fi... for D7s example.

Anonymous’s picture

update_prepare_d8_bootstrap() seems to be the only place we can fix this for language stuff.

update_fix_d8_requirements() runs too late (after drupal_language_initialize()).

so, either we add the db changes to update_prepare_d8_bootstrap()

OR

we stop calling drupal_language_initialize() before update_fix_d8_requirements().

the fixes don't look too hard once we've decided which way to go.

Gábor Hojtsy’s picture

Since these are includes used in the bootstrap, I think it would make lots of sense to fix for the upgrade path in update_prepare_d8_bootstrap().

catch’s picture

update_prepare_d8_bootstrap() seems fine to me as well.

We need to stop recommending to people that they run update.php on their live site for major version upgrades. That should always happen on a copy of the site then be copied across (regardless of backups etc. it makes no sense to run a process that is known to be extremely fragile on your live database). If we do that then the 'read only' nature of update_prepare_d8_bootstrap() is much less important.

Anonymous’s picture

Status: Active » Needs review
FileSize
1.42 KB

ok attached is some fairly ugly but working code to update references to 'includes/locale.inc' --> 'core/includes/locale.inc'.

i had to include the patch over at #1331350: [Upgrade path broken] Entity module not enabled during D8 upgrade to make this work.

still warnings and crap when using the db dump attached to the original post, but no WSODs.

Gábor Hojtsy’s picture

The patch looks good, I don't currently have time to test wether it actually fixes the update scenario. Also, should we add negotiation settings to the locale update DB at #1336170: Add locale module to upgrade tests?

Anonymous’s picture

Issue tags: +Needs tests

the patch works for the db dump attached by the OP.

we definitely need this stuff integrated in our upgrade tests in core.

Gábor Hojtsy’s picture

We can either add those to #1336170: Add locale module to upgrade tests or wait for that to be finished & committed and then add the settings here I guess.

plach’s picture

Either way

should we add negotiation settings to the locale update DB

sounds totally sensible to me.

Gábor Hojtsy’s picture

#1336170: Add locale module to upgrade tests added a basic locale database dump. We need to add negotiation settings and verify it works in the update (ie. no flames, update errors, etc). We can/should also put in a couple asserts to ensure the negotiation still works in D8, given the path changes all make it problematic.

Anonymous’s picture

FileSize
200.36 KB

ok, here's a patch with an updated core/modules/simpletest/tests/upgrade/drupal-7.filled.database.php.gz file that fails upgrade tests.

i've enabled translation module, and turned on some negotiation features, and we get the expected fail of not being able to load locale.inc.

Status: Needs review » Needs work

The last submitted patch, 1331370_11.patch, failed testing.

Anonymous’s picture

FileSize
201.47 KB

ok, here's one with the same database dump as #11 plus the fixes for the locale.inc path.

Anonymous’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

The update code looks good. It would be great to see the diff for the filled database code in uncompressed, non-binary form for review. Thanks!

Anonymous’s picture

FileSize
885.71 KB

re. #15, turns out that is hard.

not because running gunzip and diff on a couple of files is hard, but because the order of the generated statements seems to vary, so the diff is huge, even if the change is not. we need better tools here if getting changes like this to land will require manual review of the diffs.

so, i need some feedback on:

- are we going to require manual review of diffs from the unzipped dump files?
- do we need to spend some time making sure the output is as standardised as possible to make these diffs useful, via some better core code or documentation or drush/devel code?

i've attached the diff to make it clear what i'm talking about - the locale changes show up, but so do a bunch of spurious changes, like for the menu_router table etc.

Gábor Hojtsy’s picture

@beejeebus: well, its pretty hard to tell if the change is relevant or complete or meaningful if there is no textual diff to look at. So I'd say if we can make diffs work better for these files, that would be important.

catch’s picture

xjm’s picture

So is this issue blocked on #1364798: Impossible to generate meaningful diffs of upgrade db dumps, or is there a way we can work around it by posting some sort of manual data for review?

Gábor Hojtsy’s picture

@xjm: well, I would not say it is blocked, I guess we never had such a tool before and still made changes like that to the update dump.

plach’s picture

Rerolled the patch. I am almost sure there is also a function in theme.inc that is involved in this issue, I'll check and report. Meanwhile posting also the db dump without the fix to see if at least Locale is covered.

plach’s picture

The result for the test-only patch is not getting back, but tests fail (see the details link). So I'd say the DB dump cover the issue, at least wrt language negotiation.

xjm’s picture

I requeued the test-only patch on QA; it had been postponed.

Anonymous’s picture

ok, the reroll passed. lets get this in then? i can't RTBC this.

plach’s picture

Well, I still vaguely remember having this problem even with a function living in theme.inc but I'm note able to reproduce it anymore. Let's commit this. We may reopen it if additional problematic stored paths come up.

I contributed to the patch so I'd prefer someone else to RTBC it.

plach’s picture

@xjm:

Will you dare? ;)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Code looks good. Many great people worked on this, so thank you all!

(Getting this in would also enable us to close the major issue at #1357918: Missing update for language_default in language langcode update which proposed a similar change in the D8 update bootstrap to help solve some issues).

sun’s picture

Making some serious progress in #1364798: Impossible to generate meaningful diffs of upgrade db dumps to resolve the db dumps problem.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Nice. Committed/pushed to 8.x, thanks everyone.

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