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.
Comment | File | Size | Author |
---|---|---|---|
#21 | update-1331370-21-test.patch | 200.36 KB | plach |
#21 | update-1331370-21.patch | 201.81 KB | plach |
#16 | silly.diff.txt | 885.71 KB | Anonymous (not verified) |
#13 | lol-wut-upgrade.patch | 201.47 KB | Anonymous (not verified) |
#11 | 1331370_11.patch | 200.36 KB | Anonymous (not verified) |
Comments
Comment #1
Gábor HojtsyWe 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.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedupdate_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.
Comment #3
Gábor HojtsySince 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().
Comment #4
catchupdate_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.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedok 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.
Comment #6
Gábor HojtsyThe 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?
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedthe patch works for the db dump attached by the OP.
we definitely need this stuff integrated in our upgrade tests in core.
Comment #8
Gábor HojtsyWe 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.
Comment #9
plachEither way
sounds totally sensible to me.
Comment #10
Gábor Hojtsy#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.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 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.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedok, here's one with the same database dump as #11 plus the fixes for the locale.inc path.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #15
Gábor HojtsyThe 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!
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedre. #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.
Comment #17
Gábor Hojtsy@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.
Comment #18
catchI opened #1364798: Impossible to generate meaningful diffs of upgrade db dumps.
Comment #19
xjmSo 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?
Comment #20
Gábor Hojtsy@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.
Comment #21
plachRerolled 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.
Comment #22
plachThe 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.
Comment #23
xjmI requeued the test-only patch on QA; it had been postponed.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedok, the reroll passed. lets get this in then? i can't RTBC this.
Comment #25
plachWell, 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.
Comment #26
plach@xjm:
Will you dare? ;)
Comment #27
Gábor HojtsyCode 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).
Comment #28
sunMaking some serious progress in #1364798: Impossible to generate meaningful diffs of upgrade db dumps to resolve the db dumps problem.
Comment #29
catchNice. Committed/pushed to 8.x, thanks everyone.