Problem/Motivation

At DrupalCon Prague, we decided not to provide a Drupal 7 to Drupal 8 upgrade path using the database update system, and to instead provide data migration using a Drupal 8 data migration API based on the Migrate module. However, hundreds of 7.x to 8.x update hooks remain, which is confusing to core contributors and module developers. When discussing the criteria for a beta release, @catch, @Dries, @Moshe Weitzman, and myself agreed that these hooks should be removed from the codebase before beta 1 to reduce confusion.

Proposed resolution

  • Disallow upgrades from 7.x to 8.x.
  • Remove all implementations of hook_update_N() and hook_update_dependencies().
  • Remove unused API functions for the 7.x to 8.x migration path.

The approach has consensus from the Migrate module maintainers.

Remaining tasks

  • Latest patch needs code review.
  • Check for references to removed upgrade helpers in change records.
  • Post g.d.o/core announcement about filing migration issues after reviewing with core maintainers (see #36).

API changes

  • Any upgrades from 7.x to 8.x are now disallowed. The user will receive a requirements error if attempting to update with a Drupal 7 database.
  • The partial 7.x to 8.x upgrade path using the database update system is no longer provided in Drupal 8.
  • hook_update_N() functions now start at _###1 to reserve #000 for the minimum installed schema with no updates. Examples from this patch:
    • function update_test_1_update_8001()
    • function update_test_2_update_8001()
    • function update_test_3_update_8001()
    • function update_script_test_update_8001()

    A requirements error will be displayed when updating if a module defines a hook_update_8000().

  • Several update helper functions are removed.
    • function update_prepare_stored_includes()
    • function update_prepare_d8_language()
    • function update_prepage_d8_bootstrap()
    • function update_fix_d8_requirements()
    • function update_variable_get()
    • function update_variable_set()
    • function update_variable_del()
    • function update_variables_to_config()
    • function update_install_default_config()
    • function update_variables_to_state()
    • function _update_8000_entity_get_display()
    • function _update_8000_entity_get_form_display()
    • function _update_8003_field_create_field()
    • function _update_8003_field_create_instance()
    • function _update_8006_field_write_data_sql()
    • function _update_7000_node_get_types()

    Additionally, update_extra_requirements() is replaced with specific functions update_system_schema_requirements() and update_settings_file_requirements().

  • A couple constants are altered as well.
    • Removed
      • const SCHEMA_INSTALLED = 0;
      • const REQUIRED_D7_SCHEMA_VERSION = '7069';
    • Added
      • const \Drupal::CORE_MINIMUM_SCHEMA_VERSION = 8000;
CommentFileSizeAuthor
#154 Untitled.png21.06 KBKyleNakhul
#132 update-2168011-119-do-not-test.patch89.79 KBxjm
#130 update-2168011-119.patch295.79 KBjessebeach
#127 interdiff.txt17.61 KBjessebeach
#127 update-2168011-127.patch319.19 KBjessebeach
#121 pending-2.png41.17 KBjessebeach
#121 finsihed-2.png44.53 KBjessebeach
#121 update-2.png30.54 KBjessebeach
#120 interdiff.txt666 bytesznerol
#120 update-2168011-119.patch295.79 KBznerol
#118 update-2168011-118.patch295.54 KBznerol
#116 interdiff.txt410 bytesjessebeach
#116 update-2168011-116.patch301.49 KBjessebeach
#115 interdiff.txt799 bytesxjm
#115 update-2168011-114.patch301.53 KBxjm
#112 interdiff.txt1.48 KBxjm
#112 update-2168011-112.patch301.55 KBxjm
#109 interdiff.txt5.2 KBxjm
#109 update-2168011-109.patch301.74 KBxjm
#107 interdiff.txt2.53 KBxjm
#107 update-2168011-107.patch300.47 KBxjm
#104 drupal_2168011_104.patch299.43 KBXano
#101 interdiff.txt2.35 KBjessebeach
#101 update-2168011-101.patch299.21 KBjessebeach
#98 interdiff.txt9.7 KBjessebeach
#98 update-2168011-98.patch303.59 KBjessebeach
#98 update-2168011-98-fail.patch5.17 KBjessebeach
#93 update-2168011-93.patch292.18 KBxjm
#84 update-2168011-83.patch301.29 KBjessebeach
#82 update-2168011-82.patch497.13 KBjessebeach
#82 interdiff.txt10.02 KBjessebeach
#75 update-2168011-75.patch485.56 KBjessebeach
#75 interdiff.txt2.06 KBjessebeach
#73 7x_message.png23.78 KBxjm
#59 update-2168011-59.patch289.24 KBxjm
#58 interdiff.txt2.11 KBjessebeach
#58 update-2168011-58.patch485.06 KBjessebeach
#58 verify-7_x-2.png35.86 KBjessebeach
#56 Requirements-problem-2.png61.53 KBjessebeach
#52 update-errors-2.png302.82 KBjessebeach
#52 pending-updates-2.png96.56 KBjessebeach
#49 update-2168011-48-do-not-test.patch79.18 KBxjm
#49 update-2168011-48.patch483.69 KBxjm
#46 interdiff.txt8.97 KBxjm
#46 update-2168011-46.patch483.69 KBxjm
#46 update-2168011-46-FAIL.patch25.99 KBxjm
#46 what_pending_updates.png34.21 KBxjm
#42 docs-update.txt4.06 KBxjm
#42 update-2168011-42.patch482.01 KBxjm
#42 update-2168011-42-FAIL.patch25.97 KBxjm
#31 interdiff.txt31.32 KBxjm
#31 update-2168011-31-do-not-test.patch65.36 KBxjm
#31 update-2168011-31.patch469.87 KBxjm
#30 update-2168011-27-do-not-test.patch39.42 KBxjm
#29 update-2168011-27-do-not-test.patch0 bytesxjm
#29 interdiff.txt4.14 KBxjm
#29 update-2168011-27.patch444.08 KBxjm
#24 update-2168011-21-do-not-test.patch37.82 KBxjm
#21 interdiff.txt1.14 KBxjm
#21 update-2168011-21.patch442.33 KBxjm
#18 interdiff.txt32.71 KBxjm
#18 update-2168011-17.patch442.2 KBxjm
#15 interdiff.txt905 bytesxjm
#15 update-2168011-15.patch409.61 KBxjm
#14 interdiff.txt4.23 KBxjm
#14 update-2168011-14.patch409.61 KBxjm
#12 interdiff.txt2.79 KBxjm
#12 update-2168011-12.patch407.27 KBxjm
#9 interdiff.txt521 bytesxjm
#9 update-2168011-9.patch404.99 KBxjm
#7 interdiff.txt3.01 KBxjm
#7 update-2168011-7.patch404.48 KBxjm
remove_update_hooks.patch407.22 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

The order doesn't bother me at all - these are only going to be there for a while to save people looking back through git history.

xjm’s picture

Issue summary: View changes

We might also need to update some change records for the removed upgrade helpers. E.g., I ran into https://drupal.org/node/2012896.

Edit: There's also a bunch more stuff in update.inc that we should discuss what to do with.

Damien Tournoud’s picture

If we want to keep supporting minor updates (I assume we do), we need to pin the schema version of all the modules.

We needs two things:

  • When a module is installed, its schema version should be guaranteed to have a version in 8xxx, even if it has not update functions.
  • When a module in < 8xxx is present, it should not be updatable

The lowest version in 8xxx is 8000, so we should default to that. But that means that *_update_8000 needs to be banned, the first real update need to be 8001.

So we need three changes in addition to removing updates:

  • In Drupal\Core\Extension\ModuleHandler::install(), above the "last removed" handling, add a $version = max($version, \Drupal::CORE_COMPATIBILITY * 1000);;
  • In update_get_update_list(), throw an exception if an update function in 8000 exists;
  • In update_get_update_list(), above the "last removed" handling, add a check for $schema_version < \Drupal::CORE_COMPATIBILITY * 1000;
  • Add two tests to cover the requirements above (newly installed module has a schema in 8000, if a module has a schema < 8xxx, it is not updatable.

In addition to all of this, we need to decide what to do with the update tests.

xjm’s picture

Thanks @Damien Tournoud!

In addition to all of this, we need to decide what to do with the update tests.

The test coverage for the 7.x -> 8.x upgrade path is already removed; is that what you're referring to? I left update functionality self-tests alone because we're still going to use them for 8.0.0 -> 8.0.1 (and possibly 8.0-beta1 -> 8.0-beta2), etc.

xjm’s picture

xjm’s picture

Status: Needs work » Needs review
FileSize
404.48 KB
3.01 KB

Got a little overzealous in the OP.

Didn't address #3 yet.

Damien Tournoud’s picture

The test coverage for the 7.x -> 8.x upgrade path is already removed; is that what you're referring to?

Yeah. I was looking at an old checkout of 8.x.

xjm’s picture

Title: Remove all 7.x to 8.x update hooks » Remove all 7.x to 8.x update hooks and disallow updates from the previous major version
FileSize
404.99 KB
521 bytes

When I went to implement the first part of #3 (which should also fix one of the remaining test failures), I noticed that the result was that of replacing all core usage of SCHEMA_INSTALLED with N000.

diff --git a/core/includes/install.inc b/core/includes/install.inc
index 256c79e..849dd97 100644
--- a/core/includes/install.inc
+++ b/core/includes/install.inc
@@ -634,8 +634,11 @@ function drupal_install_system() {
 
   $system_path = drupal_get_path('module', 'system');
   require_once DRUPAL_ROOT . '/' . $system_path . '/system.install';
+
+  // Set the schema version to the number of the last update provided by the
+  // module, or the minimum for the core version.
   $system_versions = drupal_get_schema_versions('system');
-  $system_version = $system_versions ? max($system_versions) : SCHEMA_INSTALLED;
+  $system_version = $system_versions ? max(max($system_versions), \Drupal::CORE_COMPATIBILITY * 1000);
   \Drupal::keyValue('system.schema')->set('system', $system_version);
 
   // System module needs to be enabled and the system/module lists need to be
diff --git a/core/includes/schema.inc b/core/includes/schema.inc
index d0fd30b..6e672e4 100644
--- a/core/includes/schema.inc
+++ b/core/includes/schema.inc
@@ -20,11 +20,6 @@
 const SCHEMA_UNINSTALLED = -1;
 
 /**
- * Indicates that a module has been installed.
- */
-const SCHEMA_INSTALLED = 0;
-
-/**
  * Gets the schema definition of a table, or the whole database schema.
  *
  * The returned schema will include any modifications made by any
diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php
index 92cf2a4..429fbd2 100644
--- a/core/lib/Drupal/Core/Extension/ModuleHandler.php
+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -627,10 +627,10 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
         // Now install the module's schema if necessary.
         drupal_install_schema($module);
 
-        // Set the schema version to the number of the last update provided
-        // by the module.
+        // Set the schema version to the number of the last update provided by
+        // the module, or the minimum for the core version.
         $versions = drupal_get_schema_versions($module);
-        $version = $versions ? max($versions) : SCHEMA_INSTALLED;
+        $version = $versions ? max($versions) : \Drupal::CORE_COMPATIBILITY * 1000;

So maybe it makes sense to just re-define() SCHEMA_INSTALLED with \Drupal::CORE_COMPATIBILITY * 1000? Attached tries that. (No code for preventing updates yet, but I agree that should be in scope, so retitling accordingly.) NR for the bot.

catch’s picture

Isn't core compatibility '8.x' as a string?

xjm’s picture

Isn't core compatibility '8.x' as a string?

Haha, it totally is.

Maybe we should instead just replace SCHEMA_INSTALLED with Drupal::CORE_MINIMUM_SCHEMA_VERSION or something then?

xjm’s picture

FileSize
407.27 KB
2.79 KB

Attached does that instead (also fixes an error in the previous logic).

xjm’s picture

  1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -627,10 +627,11 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
             $versions = drupal_get_schema_versions($module);
    -        $version = $versions ? max($versions) : SCHEMA_INSTALLED;
    +        $version = $versions ? max($versions) : 0;
    +        $version = max($version, \Drupal::CORE_MINIMUM_SCHEMA_VERSION);
    

    @dawehner noticed that this is a little confusing with both $systemv_versions and $system_version, so it'd probabl be clearer to just set $version = \Drupal.... first and then go from there.

  2. +++ b/core/includes/install.inc
    @@ -634,8 +634,12 @@ function drupal_install_system() {
    +
    +  // Set the schema version to the number of the last update provided by the
    +  // module, or the minimum core schema version.
       $system_versions = drupal_get_schema_versions('system');
    -  $system_version = $system_versions ? max($system_versions) : SCHEMA_INSTALLED;
    +  $system_version = $system_versions ? max($system_versions) : 0;
    +  $system_version = max($system_version, \Drupal::CORE_MINIMUM_SCHEMA_VERSION);
    

    The fact that this is more or less a duplicate of the other hunk makes it look like it wants to be factored out, but possibly out of scope.

Waiting for the bot, though.

xjm’s picture

Issue tags: +Needs tests
FileSize
409.61 KB
4.23 KB

With update compatibility handling and some cleanup. Needs tests still though.

xjm’s picture

FileSize
409.61 KB
905 bytes

Ahem.

xjm’s picture

FileSize
442.2 KB
32.71 KB

Attached fixes some merge conflicts, plus:

  • Fixes the upgrade path test to use the new required minimum update of 8001.
  • Updates hook_update_N() documentation.
  • Uses update hook examples over 8001 in hook_update_dependencies() and hook_update_last_removed().
  • Changes update_prepare_d8_bootstrap() to throw a requirements error for a D7 database.
  • Removes the REQUIRED_D7_SCHEMA_VERSION constant.
  • Removes update_fix_d8_requirements() since it is no longer needed (D7 sites will never get past update_prepare_d8_bootstrap()).

There's still a bit of documentation that needs updating around the update API function examples (we need to actually re-think the example code). There's also a bit in UpdateModuleHandler::install() that needs updating.

FInally, we should probably remove all these unless the Migrate folks are using them:

  • update_variable_get()
  • update_variable_set()
  • update_variable_del()
  • update_variables_to_config()
  • update_variables_to_state()
  • update_add_uuids()

That could probably be done in a followup issue.

xjm’s picture

+++ b/core/includes/update.inc
@@ -148,292 +138,23 @@ function update_prepare_d8_bootstrap() {
-  // running an up-to-date version of Drupal 7 before proceeding. Note this has
-  // to happen AFTER the database bootstraps because of
-  // drupal_get_installed_schema_version().
+  // Ensure that the site is running Drupal 8 before proceeding.

Probably should keep the second sentence of the original comment here.

xjm’s picture

FileSize
442.33 KB
1.14 KB

Attached fixes that comment (and a, er, syntax error).

Damien Tournoud’s picture

Hehe. Sorry about CORE_COMPATIBILITY being a string :)

     // Skip uninstalled and incompatible modules.
-    if ($schema_version == SCHEMA_UNINSTALLED || update_check_incompatibility($module)) {
+    if ($schema_version == SCHEMA_UNINSTALLED || $schema_version < \Drupal::CORE_MINIMUM_SCHEMA_VERSION || update_check_incompatibility($module)) {
       continue;
     }

^ This is debatable. Do we want to just silently skip those modules or do we want to raise a warning like the handling of update_last_removed?

xjm’s picture

^ This is debatable. Do we want to just silently skip those modules or do we want to raise a warning like the handling of update_last_removed?

Well, the check against update_check_incompatibility() fails silently if the module's .info file refers to an older version of core or PHP, regardless of whether it was previously installed, so that's why I included it there. It seemed comparable. I could see raising a requirements warning too; I wasn't sure why that isn't already the behavior for installed modules that use an older version of core/PHP/etc. Followup maybe?

Here's a reviewable-sized patch for the functional changes up through #21, without the moved update hooks.

Damien Tournoud’s picture

Well, the check against update_check_incompatibility() fails silently if the module's .info file refers to an older version of core or PHP, regardless of whether it was previously installed, so that's why I included it there. It seemed comparable. I could see raising a requirements warning too; I wasn't sure why that isn't already the behavior for installed modules that use an older version of core/PHP/etc. Followup maybe?

The support for update_last_removed is pretty similar to what we have here: it's about a module that is compatible to the current Drupal core but has a schema version that is lower then the first update function available.

It doesn't really matter, because this scenario should be impossible with Drupal 8, but it makes sense to me that those two have the same effect.

In other words, we have two different behaviors right now:

  • If a module is incompatible with the current installation (for example because it is for a different core version or incompatible with the current version of PHP), we simply ignore it.
  • If a module is compatible with the current installation but cannot be upgraded for any reason (because we don't have an upgrade path between the old schema and the new schema in the module), we report this as a warning.
catch’s picture

It's possible to get into an update_last_removed pickle with a contrib module still. Say they have 8.x-1.x branches up to 8.x-7.x and upgrading from each requires going through each intermediate branch - then the 8.x-7.x could have an update_last_removed that is 8600, but your site is still on 8001 or whatever.

xjm’s picture

Status: Needs work » Needs review
FileSize
444.08 KB
4.14 KB
0 bytes

Attached fixes a couple of dumb typos that broke update.php (good times) and also resets a static cache in a test that worked by accident before.

The update dependency tests assume that the system module has at least three update hooks; they'll need to be reworked to rely on a different test module instead. For now I just stuck empty hooks in system.install to demonstrate that the patch works otherwise.

xjm’s picture

FileSize
39.42 KB
xjm’s picture

FileSize
469.87 KB
65.36 KB
31.32 KB

Attached fixes the update dependency tests to use another test module, and gleefully rips out the variable-to-config upgrade path, its tests, and other unused update code.

chx’s picture

So, I am OK with this approach -- we can do git mining or we are grateful we don't need to, we will be all OK. The changes that usually enter into migration is usually some stupid functionality storing their configuration in variables in some weird ways, that's all we really need to read out.

However, it is time we actually mandate migrations from people who write such changes. If there are no update hooks we do need to begin doing that or we leave some data behind. Most often, it will be like "ah yeah, this change will be covered by the node/user/term etc migration".

chx’s picture

We discussed process ongoing with xjm and got to the point where a new policy would be in place: File an issue in the core queue, migration component if you would add a hook_update_N() that actually interacts with site data rather than just the schema. We still will close most of those but this is the best we can come up with until people more understand what and how migrate is doing what it's doing.

xjm’s picture

Issue summary: View changes
Issue tags: +Avoid commit conflicts

Happy green patch. Time to tag; it's getting close.

Updated the summary and remaining tasks.

xjm’s picture

Status: Needs review » Needs work

NW for the new tests and docs updates.

xjm’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
25.97 KB
482.01 KB
4.06 KB

Attached adds the missing test coverage and updates the remaining API documentation.

Damien Tournoud’s picture

Any words on the bottom half of #27?

xjm’s picture

Status: Needs review » Needs work
Issue tags: -imp

In other words, we have two different behaviors right now:

  • If a module is incompatible with the current installation (for example because it is for a different core version or incompatible with the current version of PHP), we simply ignore it.
  • If a module is compatible with the current installation but cannot be upgraded for any reason (because we don't have an upgrade path between the old schema and the new schema in the module), we report this as a warning.

I still think somehow having a 7XXX schema installed on D8 is more like the first case, but I don't feel strongly about it. I guess I'd like to test it manually to see which felt more natural, but OTOH I can't imagine a scenario that could lead to it other than an API call manually changing the schema version. I guess informing the user is unlikely to be a bad thing.

+++ b/core/includes/schema.inc
@@ -143,7 +139,17 @@ function drupal_get_schema_versions($module) {
-    foreach ($updates as &$module_updates) {
+    foreach ($updates as $module_name => &$module_updates) {
+      // No module is allowed to define a schema update of exactly
+      // \Drupal::CORE_MINIMUM_SCHEMA_VERSION because this is reserved for the
+      // minimum installed version.
+      if (in_array(\Drupal::CORE_MINIMUM_SCHEMA_VERSION, $module_updates)) {
+        throw new ExtensionSchemaVersionException(format_string("%module_name's hook_update_N() implementations must use N greater than @version.", array(
+          '%module_name' => $module_name,
+          '@version' => \Drupal::CORE_MINIMUM_SCHEMA_VERSION,
+        )));
+      }
+

Also realized over night that throwing an exception is probably too aggressive, especially here. It'd probably just be better to have a requirements warning on both installation and update, since this could make currently ported contrib modules explode sites, for example, and having that misnamed hook in the codebase is unlikely to break anything during normal site operation unless we make it.

Marking NW for those two changes.

Also does anyone know what the proper tag for migrate-related issues is? :P

xjm’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
34.21 KB
25.99 KB
483.69 KB
8.97 KB

Attached makes those changes. I noticed a bug in the process of testing this: If there are only invalid updates, update.php does not display the error messages from update_get_update_list(), resulting in this confusing screen:

This also means that hook_update_last_removed() has insufficient test coverage (and it actually appears "insufficient" is "none" in this case). This is out of scope, though, so I'll file it as a separate issue. For now the patch uses less specific, but still valid assertions to confirm that a requirement error was raised.

Attached also fixes the comment in UpdateModuleHandler; @alexpott suggested leaving the support for stream wrappers and just making the comment generic. There's still this other bit in UpdateModuleHandler::install() that doesn't make so much sense anymore:

      // Check for initial schema and install it. The schema version of a newly                                                                            
      // installed module is always 0. Using 8000 here would be inconsistent                                                                               
      // since $module_update_8000() may involve a schema change, and we want                                                                              
      // to install the schema as it was before any updates were added.                                                                                    
      module_load_install($module);
      $function = $module . '_schema_0';
      if (function_exists($function)) {
        $schema = $function();
        foreach ($schema as $table => $spec) {
          db_create_table($table, $spec);
        }
      }
xjm’s picture

Checked with @catch about #46 -- #1929816: Remove all references to/implementations of hook_schema_0() will take care of that, so we can exclude it from the scope here.

We could probably use someone other than me testing it manually, and someone testing with a D7 database to make sure it works as intended. Other than that, I think this patch is pretty much ready.

xjm’s picture

FileSize
483.69 KB
79.18 KB

Needs a reroll already.

The -do-not-test patch shows all the changes except the actual moving of the update hooks and helpers to old_updates.txt, to make it easier to do a code review on the functional changes.

webchick’s picture

Jess gave an overview of this patch today, and one thing I was surprised about is we keep this file "old_updates.txt" hanging around in the root /core directory. That file, which is really only for a small subset of the core developer team, might very well sit there for N months/years until the D7 -> D8 upgrade path is finished, i.e. we might very well ship Drupal 8.0.0 with it, and possibly 8.1.0 as well. In the meantime, it's front-and-center to anyone who's learning Drupal 8 for the first time, for them to be confused by.

It seems to me like a better thing to do would be to take that output and upload it to a meta issue that's about making sure all of the old update functions are reflected in the migration path, and/or turn it into a body text of said issue with <strike> in it or what have you. But putting it in the actual code base feels a bit ick to me. It would also make this patch much easier to review.

jessebeach’s picture

FileSize
96.56 KB
302.82 KB

I ran a 7.x to 8.x upgrade just to baseline. Those steps were:

  1. Install a 7.x site
  2. git co 8.x
  3. Set perms on settings.php to 755
  4. Navigate to /core/update.php
  5. Follow the steps in the wizard

After the requirements check page, I get a page listing the number of pending updates. 169 updates are listed.

If I run the updates, the process finishes with numerous errors, but nothing fatal:

webchick’s picture

Just spoke with chx about #51, and he's +1. Basically, what he can do is create a branch in the migrate sandbox that's from the commit before the commit of this patch for migrate volunteers to browse as a reference implementation.

Then they can do a one-time run-through of all of these at https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AgTfFkG3LW9idG... in order to catalog what needs to be accounted for and where to find it.

So here's to < 400K patches in the future. :D

jessebeach’s picture

In system.install, do we want to delete this block of code, since we shouldn't even be getting this far in the update process?

// Ensure that if upgrading from 7 to 8 we have no disabled modules.
if ($phase == 'update' && db_table_exists('system')) {
  $modules = db_query('SELECT name, info FROM {system} WHERE type = :module AND status = 0 AND schema_version <> :schema_uninstalled', array(
    ':module' => 'module',
    ':schema_uninstalled' => SCHEMA_UNINSTALLED,
  ))->fetchAllKeyed(0, 1);
  array_walk($modules, function (&$value, $key) {
    $info = unserialize($value);
    $value = $info['name'];
  });
  if (!empty($modules)) {
    $requirements['disabled_modules'] = array(
      'severity' => REQUIREMENT_ERROR,
      'title' => t('Disabled modules'),
      'value' => format_plural(count($modules), 'The %modules module is disabled.', 'The following modules are disabled: %modules', array('%modules' => implode(', ', $modules))),
      'description' => t('Drupal 8 no longer supports disabled modules. Please either enable or uninstall them before upgrading.'),
    );
  }
}
jessebeach’s picture

I've been trying to upgrade from 7.x to 8.x with the patch in #49. I'm getting a WSOD and no error in the PHP log. I've widdled the exception down to this function and specifically, this line: \Drupal::state()->set('system.module.files', $files); in system.module.

function system_rebuild_module_data() {
  $modules_cache = &drupal_static(__FUNCTION__);
  // Only rebuild once per request. $modules and $modules_cache cannot be
  // combined into one variable, because the $modules_cache variable is reset by
  // reference from system_list_reset() during the rebuild.
  if (!isset($modules_cache)) {
    $modules = _system_rebuild_module_data();
    $files = array();
    ksort($modules);
    // Add name, status, weight, and schema version.
    $installed_modules = (array) \Drupal::config('system.module')->get('enabled');
    foreach ($modules as $module => $record) {
      $record->name = $module;
      $record->weight = isset($installed_modules[$module]) ? $installed_modules[$module] : 0;
      $record->status = (int) isset($installed_modules[$module]);
      $record->schema_version = SCHEMA_UNINSTALLED;
      $files[$module] = $record->filename;
    }
    $modules = \Drupal::moduleHandler()->buildModuleDependencies($modules);
    $modules_cache = $modules;

    // Store filenames to allow system_list() and drupal_get_filename() to
    // retrieve them without having to rebuild or scan the filesystem.
    \Drupal::state()->set('system.module.files', $files);
  }
  return $modules_cache;
}
jessebeach’s picture

FileSize
61.53 KB

If I comment out the set state line mentioned in #55 above,

\Drupal::state()->set('system.module.files', $files);

then the request returns and I get a requirements page indicating that my DB is from Drupal 7 and I need to migrate to continue.

jessebeach’s picture

Ok, so my guess here is, we're running requirements checks, one of which -- system_requirements -- tries to cache a list of files...which requires a database. And since we're moving from 7.x to 8.x, we don't have a valid DB in this case.

jessebeach’s picture

FileSize
35.86 KB
485.06 KB
2.11 KB

I've introduced a function update_migrate_requirements that does the 7.x to 8.x check. If that check return a requirement array, then we don't check any other requirements and instead skip right to printing out that one to the screen.

xjm’s picture

FileSize
289.24 KB

Awesome, thanks @jessebeach! That looks like more what was supposed to happen. ;)

Attached just rerolls #58 to remove old_updates.txt. This is now only maybe the fifth largest patch I've worked on in the past year and a half.

The last submitted patch, 58: update-2168011-58.patch, failed testing.

(Just freeing up a testbot since #59 is functionally the same as #58.)

Damien Tournoud’s picture

Status: Needs review » Needs work

Ok, so my guess here is, we're running requirements checks, one of which -- system_requirements -- tries to cache a list of files...which requires a database. And since we're moving from 7.x to 8.x, we don't have a valid DB in this case.

This is obviously a bug in system_requirements(): both $phase = 'install' and $phase = 'update' need to be safe to run with the minimal install / update environment.

So this is just wrong and need to be fixed:

  if ($phase != 'install') {
    $filesystem_config = \Drupal::config('system.file');
    $directories = array(
      PublicStream::basePath(),
      
      // By default no private files directory is configured. For private files
      // to be secure the admin needs to provide a path outside the webroot.
      $filesystem_config->get('path.private'),
      file_directory_temp(),
    );
  }
xjm’s picture

Re: #63, isn't that out of scope here as an existing bug in HEAD, one that does not need to be resolved to fix this issue?

xjm’s picture

For reference, #63 refers to line 309 of system.install.

Here's the same hunk in D7:

 // Test files directories.
  $directories = array(
    variable_get('file_public_path', conf_path() . '/files'),
    
    // By default no private files directory is configured. For private files
    // to be secure the admin needs to provide a path outside the webroot.
    variable_get('file_private_path', FALSE),
  );

It used a variable then, too. Is the difference that variable_get() returns nothing when $conf is not set, meaning the default would be used, but the D8 version has no such fallback?

catch’s picture

It's not just the lack of fallback, it's that variable_get() has no inherent dependency on variable storage.

Which makes me wonder why these are calling out to config at all, should be settings after #1856766: Convert file_public_path to the new settings system (make it not configurable via UI or YAML) no?

Damien Tournoud’s picture

The problem is actually here:

  // Display an error if a newly introduced dependency in a module is not resolved.
  if ($phase == 'update') {
    $profile = drupal_get_profile();
    $files = system_rebuild_module_data();
    foreach ($files as $module => $file) {
      // Ignore disabled modules and installation profiles.
      if (!$file->status || $module == $profile) {
        continue;

  [...]

It works in D7 because we have the requirements update running before running requirement checks.

Based on this, I'm going to agree with @jessebeach solution: we only run requirement checks if we are already on D7.

Damien Tournoud’s picture

This said, could we refactor this so that we don't have this weird indirect store/get pattern?

 /**
+ * Returns and stores migration requirements that apply during the
+ * update process.
+ */
+function update_migrate_requirements($requirements = NULL) {
 }
catch’s picture

jessebeach’s picture

This said, could we refactor this so that we don't have this weird indirect store/get pattern?

Damien, I'd love to get your opinion on best practice for a better pattern here. I'm not exactly an expert on PHP at this level.

chx’s picture

> And since we're moving from 7.x to 8.x, we don't have a valid DB in this case.

What are we talking about? Did I miss something? You are not supposed to do that any more. You are supposed to install a fresh standard D8 and run migrate against the D7 database. If you point Drupal 8 to a Drupal 7 database it will work as well as if you pointed it to a phpBB database -- not at all.

jessebeach’s picture

What are we talking about? Did I miss something?

More likely I missed something :) I was just making a guess as to why it was failing, but I don't know how to debug any further than I did.

xjm’s picture

FileSize
23.78 KB

Right, this is what is supposed to happen (and now does happen) if you try to use a D7 database:

:)

(Though, I just noticed, we need to remove the "please" per our UI text guidelines.)

xjm’s picture

+++ b/core/includes/update.inc
@@ -158,282 +147,16 @@ function update_prepare_d8_bootstrap() {
+        'description' => 'Please migrate your Drupal 7 installation to a Drupal 8 installation using the Migrate module. Updating directly from Drupal 7 to Drupal 8 is not supported.',

That's where. :)

jessebeach’s picture

Status: Needs work » Needs review
FileSize
2.06 KB
485.56 KB

This patch update addresses the refactor from #68 and the 'Please' from #73.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll; not applying.

Also, we should not use drupal_static() here. There's no reason that this should be clearable on a cache clear. I think what DamZ means is to make the code simpler -- I'd start by just inlining the contents of the function in the two places that it's called, and then simplify/refactor from there. I'll look at it again later today once I'm off the phone.

amateescu’s picture

@jessebeach, our "usual" store/get pattern is drupal_set_*()/drupal_get_*(); see for example drupal_get_title() and drupal_set_title(). I *think* that's what Damien meant in #68.

In this case it would mean:
- rename update_extra_requirements() to update_set_extra_requirements()
- add a new update_get_extra_requirements() that just does return update_set_extra_requirements();
- same for update_migrate_requirements()

Also, I'm not sure the change to use drupal_static() is needed, but if you want to keep it, you can pass a default value of array() as a second argument instead of those ifs.

xjm’s picture

I really don't think we need so many helper functions for a few-line check, unless I'm missing something? Maybe I haven't understood completely. :) It doesn't seem like we ever need this as API outside this particular hunk of code.

jessebeach’s picture

Ok, I've rolled everyone's comments together in my brainmeat and I'm trying again.

jessebeach’s picture

FileSize
10.02 KB
497.13 KB

It turns out that the two functions update_extra_requirements and update_migrate_requirements were just wrapping one check each. So I've given each check its own function and just call them directly now from update_check_requirements.

This patch is literally going stale as I'm writing it. Folks are committing hook_update functions at a hot clip.

webchick’s picture

Folks are committing hook_update functions at a hot clip.

Wait. Really? They better not be! :)

jessebeach’s picture

Status: Needs work » Needs review
FileSize
301.29 KB

Sorry, the old_updates.txt snuck back in there. Removed. No other changes to the code.

jessebeach’s picture

We already had conflicts in locale.install and user.install in the past 24 hours.

Damien Tournoud’s picture

Thanks for the simplified code flow. From a visual inspection, #84 looks ready to go.

Damien Tournoud’s picture

Status: Needs review » Needs work

Actually, this is really far from being ready.

We are still calling update_prepare_d8_bootstrap() early in update.php. This fiddles with the Drupal 7 database and settings.php and it is the only reason we currently get far enough to display the error message.

I can think of two ways to move from there:

  • Wrap the drupal_bootstrap() into a try/catch block and display a manual error message like we do if version_compare(PHP_VERSION, '5.3.10') < 0;
  • Bootstrap a minimal container first (I think we have one for the installation?) check the requirements in there, and only then bootstrap the real container
jessebeach’s picture

I do not immediately know how to proceed with Damien's comments in #87. I am also looking at a couple other issues now as well. So in the interest of maintaining momentum here, Damien, would you be able to update the patch with your proposal? If not, I encourage others to attempt to incorporate this feedback into the existing patch. I will probably return to this issue in a few days, but I want to make sure that others aren't waiting on me to respond in the between time.

xjm’s picture

Status: Needs work » Needs review
FileSize
292.18 KB

I also do not understand #87. @DamZ, are you saying that we should remove update_prepare_d8_bootstrap()?

Meanwhile, here's just a reroll. It applies to 2a228e60a.

Damien Tournoud’s picture

@xjm: yes, we cannot ship with update_prepare_d8_bootstrap(), because it *modifies* your database and settings.php (among other things).

catch’s picture

Wrap the drupal_bootstrap() into a try/catch block and display a manual error message like we do if version_compare(PHP_VERSION, '5.3.10') < 0;

Just this would be enough for me - as long as we give some kind of message and direct people where to go.

star-szr’s picture

Adding a related major that maybe can be closed as a duplicate now?

Edit: Or at least it should be looked at, I found it digging around on the last page of major tasks.

xjm’s picture

@Cottser, good find! I closed it as a duplicate.

jessebeach’s picture

I've inserted a very simple check for <8000 schema version in the system table in update.inc

function update_prepare_d8_bootstrap() {
  include_once __DIR__ . '/install.inc';
  include_once __DIR__ . '/schema.inc';
  // Bootstrap to configuration.
  drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);

  // Bootstrap the database.
  require_once __DIR__ . '/database.inc';

  // Updating from a site schema version prior to 8000 should block the update
  // process. Ensure that the site is not attempting to update a database
  // created in a previous version of Drupal.
  try {
    $system_schema = db_query('SELECT schema_version FROM {system} WHERE name = :system', array(':system' => 'system'))->fetchField();
  }
  catch (\Exception $e) {
    // The schema_version does not exist in the system table, so continue
    // for the moment assuming that this is a Drupal 8 database schema. Another
    // check will be made later to verify this is the case.
  }
  if (isset($system_schema) && $system_schema < \Drupal::CORE_MINIMUM_SCHEMA_VERSION) {
    print 'Your system schema version is ' . $system_schema . '. Updating directly from a schema version prior to 8000 is not supported. You must <a href="https://drupal.org/node/2179269">migrate your site to Drupal 8</a> first.';
    exit;
  }
...

I'm fairly certain that this runs before we start manipulating the database. I've run this check as soon as is possible given that we need to run a db_query so database.inc needs to be loading and that file requires some configuration in order to establish a DB connection.

I've left in the later requirements check for the schema during the requirements gathering phase. The most we can determine early on is if a system table exists and if it has a schema_version field and if the value of this field is less than 8000. If none of that is true, then we don't necessarily have a Drupal 8 database; we just know we don't have a Drupal 7 database.

I also added a test that mocks a Drupal 7 database and asserts that the error message is printed. The -fail patch file includes just this test.

I think this should cover all the commentary so far.

Damien Tournoud’s picture

@jessebeach Good! But the rest of update_prepare_d8_bootstrap() (modifying settings.php, installing the config directories, etc.) is unnecessary now and needs to go away too.

jessebeach’s picture

FileSize
299.21 KB
2.35 KB

Alright, this update removes the unnecessary manipulation of the settings file and config directories.

moshe suggests that we an go further with the refactoring of update.php, perhaps even turning it into a controller on a route rather than a top-level page that needs to manage a bootstrap.

I'm personally a little hesitant to go down that road in this issue especially since it feels like we're right about to resolve this critical. Refactor would be a very agreeable nice-to-have follow-up.

xjm’s picture

Agreed, let's file a followup for that, since it should not block a beta.

webchick’s picture

Status: Needs review » Needs work

No longer applies. :( Looks like #2121849: Move file_chmod_directory and file_chmod_file out of config and into settings so that drush_chmod() is independant of the config system is the culprit. It's only a major, but it does unblock Drush so I'd rather not roll it back.

Xano’s picture

Status: Needs work » Needs review
FileSize
299.43 KB

Re-roll because of system.install.

xjm’s picture

FileSize
300.47 KB
2.53 KB

Cleaning up a couple other stale references. (The UpdateModuleHandler code will be removed in #1929816: Remove all references to/implementations of hook_schema_0(), but the reference is just confusing now so best to simply remove it.)

I confirmed that all hook_update_N() for core are still removed, and all the remaining references to *_update_8NNN and hook_update_N() have updated documentation.

xjm’s picture

Here's update_prepare_d8_bootstrap() as of the current patch:

/**                                                                                                                                              
 * Performs extra steps required to bootstrap when using a Drupal 7 database.                                                                    
 *                                                                                                                                               
 * Users who still have a Drupal 7 database (and are in the process of                                                                           
 * updating to Drupal 8) need extra help before a full bootstrap can be                                                                          
 * achieved. This function does the necessary preliminary work that allows                                                                       
 * the bootstrap to be successful.                                                                                                               
 *                                                                                                                                               
 * No access check has been performed when this function is called, so no                                                                        
 * irreversible changes to the database are made here.                                                                                           
 */
function update_prepare_d8_bootstrap() {
  include_once __DIR__ . '/install.inc';
  include_once __DIR__ . '/schema.inc';
  // Bootstrap to configuration.                                                                                                                 
  drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);

  // Bootstrap the database.                                                                                                                     
  require_once __DIR__ . '/database.inc';

  // Updating from a site schema version prior to 8000 should block the update                                                                   
  // process. Ensure that the site is not attempting to update a database                                                                        
  // created in a previous version of Drupal.                                                                                                    
  try {
    $system_schema = db_query('SELECT schema_version FROM {system} WHERE name = :system', array(':system' => 'system'))->fetchField();
  }
  catch (\Exception $e) {
    // The schema_version does not exist in the system table, so continue                                                                        
    // for the moment assuming that this is a Drupal 8 database schema. Another                                                                  
    // check will be made later to verify this is the case.                                                                                      
  }
  if (isset($system_schema) && $system_schema < \Drupal::CORE_MINIMUM_SCHEMA_VERSION) {
    print 'Your system schema version is ' . $system_schema . '. Updating directly from a schema version prior to 8000 is not supported. You must <a href="https://drupal.org/node/2179269">migrate your site to Drupal 8</a> first.';
    exit;
  }

  // Enable UpdateServiceProvider service overrides.                                                                                             
  // @see update_flush_all_caches()                                                                                                              
  $GLOBALS['conf']['container_service_providers']['UpdateServiceProvider'] = 'Drupal\Core\DependencyInjection\UpdateServiceProvider';
  $GLOBALS['conf']['update_service_provider_overrides'] = TRUE;

  // module.inc is not yet loaded but there are calls to module_config_sort()                                                                    
  // below.                                                                                                                                      
  require_once __DIR__ . '/module.inc';

  // Now remove the cache override.                                                                                                              
  $settings = settings()->getAll();
  new Settings($settings);
  $kernel = new DrupalKernel('update', drupal_classloader(), FALSE);
  $kernel->boot();

  // Clear the D7 caches, to ensure that for example the theme_registry does not                                                                 
  // take part in the upgrade process.                                                                                                           
  Drupal::cache('cache')->deleteAll();
}

I think we probably want to move or get rid of those last hunks as well, remove the function entirely, and move the try/catch up into update.php?

xjm’s picture

FileSize
301.74 KB
5.2 KB

Attached moves most of what remained of update_prepare_d8_bootstrap() directly into update.php and removes a D7 upgrade-specific bit from #1886448: Rewrite the theme registry into a proper service. I think the rest is a required part of bootstrapping to run updates (and therefore I'm not entirely sure why it was in update_prepare_d8_bootstrap(), but any further refactoring or cleanup needn't block a beta, I think).

Berdir’s picture

+++ b/core/update.php
@@ -343,8 +297,44 @@ function update_check_requirements($skip_warnings = FALSE) {
+}
...
+
+// Updating from a site schema version prior to 8000 should block the update
+// process. Ensure that the site is not attempting to update a database
+// created in a previous version of Drupal.
+try {
+  $system_schema = db_query('SELECT schema_version FROM {system} WHERE name = :system', array(':system' => 'system'))->fetchField();
+}
+catch (\Exception $e) {
+  // The schema_version does not exist in the system table, so continue
+  // for the moment assuming that this is a Drupal 8 database schema. Another
+  // check will be made later to verify this is the case.
+}
+if (isset($system_schema) && $system_schema < \Drupal::CORE_MINIMUM_SCHEMA_VERSION) {
+  print 'Your system schema version is ' . $system_schema . '. Updating directly from a schema version prior to 8000 is not supported. You must <a href="https://drupal.org/node/2179269">migrate your site to Drupal 8</a> first.';
+  exit;

I think this would be easier to follow if the if below is within the try block, as you don't need the isset($system_schema) then and the code that belongs together is together.

That said, I'm wondering if a db_table_exists() + a db_query() would be better, to avoid exceptions for the expected code flow.

xjm’s picture

Makes sense. Rerolling for that.

xjm’s picture

FileSize
301.55 KB
1.48 KB
Berdir’s picture

+++ b/core/update.php
@@ -308,17 +308,12 @@ function update_task_list($active = NULL) {
 // created in a previous version of Drupal.
-try {
+if (db_table_exists('system')) {
   $system_schema = db_query('SELECT schema_version FROM {system} WHERE name = :system', array(':system' => 'system'))->fetchField();
-}
-catch (\Exception $e) {
-  // The schema_version does not exist in the system table, so continue
-  // for the moment assuming that this is a Drupal 8 database schema. Another
-  // check will be made later to verify this is the case.
-}
-if (isset($system_schema) && $system_schema < \Drupal::CORE_MINIMUM_SCHEMA_VERSION) {
-  print 'Your system schema version is ' . $system_schema . '. Updating directly from a schema version prior to 8000 is not supported. You must <a href="https://drupal.org/node/2179269">migrate your site to Drupal 8</a> first.';
-  exit;
+  if (isset($system_schema) && $system_schema < \Drupal::CORE_MINIMUM_SCHEMA_VERSION) {
+    print 'Your system schema version is ' . $system_schema . '. Updating directly from a schema version prior to 8000 is not supported. You must <a href="https://drupal.org/node/2179269">migrate your site to Drupal 8</a> first.';
+    exit;
+  }

Tiny nitpick: The isset() is no longer necessary, even if the query would not find anything, it's explicitly set to NULL and you can compare it.

xjm’s picture

I tend toward cautious when it comes to loose typing, but:

[tesla:drupal | Sun 17:23:45] $ php -r "print NULL < 8000;"
1

So fixed in attached. :P

jessebeach’s picture

FileSize
301.49 KB
410 bytes

There was a stray comment that no longer applied. I removed it since the code is fairly self-explanatory here: load settings and bootstrap the kernel.

- // Now remove the cache override.

I read through the changes again and it's looking great. I think we're ready to get this in!

Berdir’s picture

Almost, needs a re-roll first, #2167109: Remove Variable subsystem got in and has a few places that conflict with this, although I tried to minimize the overlaps :)

znerol’s picture

FileSize
295.54 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 118: update-2168011-118.patch, failed testing.

znerol’s picture

Status: Needs work » Needs review
FileSize
295.79 KB
666 bytes

Initialization of the request somehow got lost in the last reroll. Hope this will turn green again.

jessebeach’s picture

FileSize
30.54 KB
44.53 KB
41.17 KB

Updating from 7.x to 8.x is properly blocked.

7.x to 8.x update is blocked with an ugly message in a string, no theming

Updating from 8.x^ to 8.x functions as expected.

pending updates screen in 8.x update, no errors

8.x updates result screen, no errors

I think we're ready to get this in. Any objections?

Berdir’s picture

See #2178581: Add an AnonymousUserSession object in favour of using drupal_anonymous_user(), what should we do about the generate-dX-content.sh scripts? I think they're kinda useless as migrate tests afaik work differently and don't need them and as assuming over there, they are very likely hopelessly broken anyway.

Berdir’s picture

120: update-2168011-119.patch queued for re-testing.

jessebeach’s picture

what should we do about the generate-dX-content.sh scripts? I think they're kinda useless as migrate tests afaik work differently and don't need them and as assuming over there, they are very likely hopelessly broken anyway.

I can't speak to the veracity of this, but I trust berdir (and in general :) ) here. But the question for this issue is, can we cleave off a non-Critical sub-issue to deal with generate-dX-content.sh and get this Critical closed? If the feeling is no, then I'll look into berdir's comment.

catch’s picture

I think we can look at that in a follow-up.

xjm’s picture

chx said those were useful for migrate, but that was awhile ago. And yes, let's open a followup for anything not in the current scope, please. :)

jessebeach’s picture

FileSize
319.19 KB
17.61 KB

Major followup: #2185565: Figure out what to do with dump-database-d*.sh and generate-d*-content.sh scripts.

From #drupal-contrib:

berdir 12:25 jbeach: sure, np. chx told me that they don't need the generate, but they need the dump scripts

So I've removed the generate scripts from /core/scripts.

Alrighty then, I think we're good to go.

jessebeach’s picture

Also, the diff --stat, which I think is fun in this case :)

core/modules/locale/locale.install                 |  584 ----------
 core/modules/menu/menu.install                     |   96 --
 core/modules/node/node.install                     |  704 ------------
 core/modules/rdf/rdf.install                       |   60 --
 core/modules/search/search.install                 |   92 --
 core/modules/shortcut/shortcut.install             |  183 ----
 core/modules/simpletest/simpletest.install         |   13 -
 core/modules/statistics/statistics.install         |   41 -
 core/modules/syslog/syslog.install                 |   21 -
 .../Tests/Update/DependencyHookInvocationTest.php  |    8 +-
 .../system/Tests/Update/DependencyMissingTest.php  |   10 +-
 .../system/Tests/Update/DependencyOrderingTest.php |   14 +-
 .../system/Tests/Update/InvalidUpdateHook.php      |   63 ++
 .../system/Tests/Update/UpdateScriptTest.php       |  128 ++-
 .../Drupal/system/Tests/Update/UpdatesWith7x.php   |   63 ++
 core/modules/system/system.api.php                 |   86 +-
 core/modules/system/system.install                 | 1131 --------------------
 .../update_script_test/update_script_test.install  |   25 +-
 .../modules/update_test_0/update_test_0.info.yml   |    7 +
 .../modules/update_test_0/update_test_0.install    |   24 +
 .../modules/update_test_0/update_test_0.module     |    1 +
 .../modules/update_test_1/update_test_1.install    |   43 +-
 .../modules/update_test_2/update_test_2.install    |   32 +-
 .../modules/update_test_3/update_test_3.install    |    8 +-
 .../update_test_invalid_hook.info.yml              |    7 +
 .../update_test_invalid_hook.install               |   12 +
 .../update_test_invalid_hook.module                |    1 +
 .../update_test_with_7x.info.yml                   |    7 +
 .../update_test_with_7x.install                    |   25 +
 .../update_test_with_7x/update_test_with_7x.module |    1 +
 core/modules/taxonomy/taxonomy.install             |  262 -----
 core/modules/text/text.install                     |   17 -
 core/modules/toolbar/toolbar.install               |   27 -
 core/modules/tracker/tracker.install               |   48 -
 core/modules/update/update.install                 |   35 -
 core/modules/user/user.install                     |  806 --------------
 core/scripts/generate-d6-content.sh                |  206 ----
 core/scripts/generate-d7-content.sh                |  318 ------
 core/update.php                                    |   89 +-
 60 files changed, 603 insertions(+), 7954 deletions(-)
xjm’s picture

Um, hang on. Let's please not introduce that change here; it's out of scope.

jessebeach’s picture

FileSize
295.79 KB

Ok.

I added mention of the generate-d*-content.sh scripts to #2185565: Figure out what to do with dump-database-d*.sh and generate-d*-content.sh scripts.

Attached is the patch from #119.

webchick’s picture

Yeah, agreed. I'm not sure I agree that we can remove them. It's fine if Migrate doesn't use them, but we will still need test coverage of updates in core once we start adding them.

/me refreshes https://qa.drupal.org/pifr/test/717138 every 8 seconds. ;)

xjm’s picture

Issue summary: View changes
FileSize
89.79 KB

For code review sanity, here's a patch with only the functional changes, doc updates, and added/revised test coverage.

xjm’s picture

+++ b/core/includes/update.inc
@@ -82,653 +72,95 @@ function update_check_incompatibility($name, $type = 'module') {
+function update_check_requirements($skip_warnings = FALSE) {

+++ b/core/update.php
@@ -285,53 +286,6 @@ function update_task_list($active = NULL) {
-function update_check_requirements($skip_warnings = FALSE) {

Note that the function is moved, not removed.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
catch’s picture

Status: Needs review » Reviewed & tested by the community

I had this open in a tab and it wasn't RTBC or committed this morning, let's get it to RTBC.

jessebeach’s picture

Change record draft: https://drupal.org/node/2186315

It's blank, but there's the stub. I can fill it out later this afternoon.

xjm’s picture

+++ b/core/includes/update.inc
@@ -985,6 +417,12 @@ function update_get_update_list() {
+      $ret[$module]['warning'] = '<em>' . $module . '</em> module cannot be updated. Its schema version is ' . $schema_version . ', which is from an earlier major release of Drupal. You will need to <a href="https://drupal.org/node/2127611">migrate the data for this module</a> instead.';

We need to replace this with a link to https://drupal.org/upgrade (or whatever) once that page is updated to actually reflect D8 migration stuff.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

All right, awesome. Jess walked me through this patch for a good 30-40 minutes.

The one thing that did mess me up was the "you can't name an update function _8000 anymore" stuff. Because as a "consumer" of the upgrade API, I never realized that that was special in any way. It does make a lot of later checks a lot easier, though, so makes sense. Let's just be sure to call this out in the change notice very strongly, because that's going to mess some folks up.

I also spotted at least one place where we're giving an end-user facing message and linking off to dev docs. We need to not do that, and we need to make sure there's a place for those people to go, so we need a major follow-up for that.

Otherwise, this looks great, AWESOME work on this everyone!!

Committed and pushed to 8.x. WOOHOO!

xjm’s picture

Change record published: https://drupal.org/node/2186315

Yay!

xjm’s picture

xjm’s picture

xjm’s picture

And, finally, a g.d.o/core announcement on what to do instead of adding hook_update_N() to D8 patches: https://groups.drupal.org/node/402803

hass’s picture

Is there any documentation what I need to do in a module to migrate variables? Cannot find anything helpful.

tim.plunkett’s picture

See core/modules/migrate_drupal/config/migrate.migration.d6_node_settings.yml for an example. That takes the variable node_admin_theme and makes it the use_admin_theme key in the node.settings.yml file.

xjm’s picture

hass’s picture

Thanks tim, will look into it.

@Xjm: i found this and have not understood anything. There are zero examples or I'm too stupid to understand them. This is not a migration guide and the changelog does not contain any instructions. This should be changed.

webchick’s picture

I think https://drupal.org/node/2141805 is what's being asked for here. It's possible this should be combined, or at least cross-linked, to the change notice for this issue.

hass’s picture

Yeah, thats a lot better. However I'm missing where I need to save the migration files in my module and I'm confused by these D6 naming... There is no D7? Is the d7 migration not yet in core?

xjm’s picture

@hass, maybe you could help make the API doc page better based on https://drupal.org/node/2141805 and the example @tim.plunkett gave?

And yes, the D6 migration is being developed first, then the D7. (Neither are complete yet.)

I updated https://drupal.org/node/2186315.

alexpott’s picture

chx’s picture

We have an API documentation but not a howto (yet?). While we do not really have a good variable-to-config migration guide written we do have one for entities https://groups.drupal.org/node/387488 which describes in painstaking detail on where to put a YAML and so on. Together with the gazillion existing variable-to-config migrations this I hope will be helpful.

Status: Fixed » Closed (fixed)

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

KyleNakhul’s picture

FileSize
21.06 KB

Can someone there help how to address this issue. I am trying to upgrade from version 8.21 to 8.2.4 and I'm the following error message when running www.mydomain.com/update.php. It's on a live server and I can't use Drush. Any help will be highly appreciated.

Here is the error

The installed schema version does not meet the minimum.
Your system schema version is -1. Updating directly from a schema version prior to 8000 is not supported. You must migrate your site to Drupal 8 first.