Ensure that ConfigImport is taking place against the same code base

I love the idea of this patch. At first I thought there were some flaws in it, but after reading it 3 times it seems very solid

This will even remove the discipline that you now need but can missed easily when syncing from dev to live or the other way around - I even guess people will be 'annoyed' in the beginning at first when this patch lands. However, it will ensure config integrity (and data integrity too in the end).

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
   @@ -97,6 +116,23 @@ protected function validateModules(ConfigImporter $config_importer) {
   +    $existing_modules = array_keys(array_intersect_key($core_extension['module'], $config_importer->getStorageComparer()->getTargetStorage()->read('core.extension')['module']));
   +    foreach ($existing_modules as $module) {
   +      if ($core_extension['versions'][$module]['current'] !== $module_data[$module]->info['version']) {
   

   Could probably use a comment, something like 'Only compare existing modules' version and schema'. I missed the variable name and array_intersect_key twice making me conclude this code had some very annoying consequences, but in the end it's not :)

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
   @@ -160,6 +202,16 @@ protected function validateThemes(ConfigImporter $config_importer) {
   +    $existing_themes = array_keys(array_intersect_key($core_extension['theme'], $config_importer->getStorageComparer()->getTargetStorage()->read('core.extension')['theme']));
   +    foreach ($existing_themes as $theme) {
   +      if ($core_extension['versions'][$theme]['current'] !== $theme_data[$theme]->info['version']) {
   

   same here

3. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -36,6 +36,9 @@ public function parse($filename) {
   +      if (!isset($parsed_info['version'])) {
   +        $parsed_info['version'] = 'None';
   +      }
   

   Interesting, I always thought that we defaulted to the current version of Drupal, but it's not (even in 7 it doesn't).

   Which makes me wonder whether we should enforce modules to have a version number anyways, instead of 'None' which is kind of pointless to me, even for a custom module. Should probably be a different issue though.

4. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -414,7 +429,11 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +        ->clear("versions.$module")
   

   Should we have an explicit test for clearing the module from versions ? I don't see one immediately (although I see unsets further down the patch) ?

5. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
   @@ -248,7 +255,9 @@ public function uninstall(array $theme_list) {
   +        ->clear("versions.$key");
   

   Same for themes

6. +++ b/core/modules/simpletest/src/Tests/KernelTestBaseTest.php
   @@ -334,6 +334,18 @@ public function testDrupalGetProfile() {
      /**
   +   * @covers ::enableModules
   +   */
   +  public function testEnableModules() {
   +    $this->assertFalse(\Drupal::moduleHandler()->moduleExists('system'));
   

   This is tested twice in the same test file - unless I'm missing some context here.

Two things that don't bother me, just pasting them here though for reference.

1. +++ b/core/config/schema/core.extension.schema.yml
   @@ -14,3 +14,16 @@ core.extension:
   +            label: 'Currently installed version'
   

   just for more clarity, something like 'Currently installed extension version' ? No biggy though.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -294,6 +294,20 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +      // There will be a new config factory.
   

   Somehow I started quoting 'There will be blood' to myself. Not sure if we really need this comment here.

Other than that, this looks ready to me. Not sure if we want more eyes on this. I can RTBC it so other commiters can take a look at it or so you can all discuss it at least ?

If I understand this issue correctly, this will force users to execute updb before running config-import.

I like this (it should be one way or the other, I don't care which).

However, what if I need to write an update that requires the config import to have been run? For instance, what if my config creates an entity bundle, and then my update script creates a new entity of that bundle?

In general I would like to see how this affects work on multiple feature branches on your project. I almost always switch between feature branches, then do a drush cim and continue. When any branch would have had database updates applied, I would rather have to use DB dumps to switch branches? I just try to understand what this would mean on a day to day workflow.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
   @@ -97,6 +116,25 @@ protected function validateModules(ConfigImporter $config_importer) {
   +          'The schema version of %module module on the source site (@source_schema) does not match this site (@target_schema).',
   

   Should we mention to run update.php?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
   @@ -115,6 +153,12 @@ protected function validateModules(ConfigImporter $config_importer) {
   +        $config_importer->logError($this->t(
   +          'Unable to install the %module module since the installed version (@config_version) does not match the code base (@code_version).',
   +          ['%module' => $module, '@config_version' => $core_extension['versions'][$module]['current'], '@code_version' => $module_data[$module]->info['version']]
   +        ));
   

   I fear a bit that some updates for sites might be more tricky, given that they deploy quite late. In this case it could happen that an update and an uninstall falls into the same import. Maybe though this restriction is fine.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -294,6 +294,20 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +      // There will be a new config factory.
   +      $extension_config = \Drupal::configFactory()->getEditable('core.extension');
   +      // Update version information in configuration.
   +      $list = $this->moduleHandler->getModuleList();
   +      $versions = $extension_config->get('versions');
   +      foreach ($modules_installed as $module_name) {
   +        $info = \Drupal::service('info_parser')->parse($list[$module_name]->getPathname());
   +        $versions[$module_name] = [
   +          'current' => $info['version'],
   +          'schema' => (string) drupal_get_installed_schema_version($module_name),
   +        ];
   +      }
   +      ksort($versions);
   +      $extension_config->set('versions', $versions)->save();
   

   What about adding this into a protected method and give it a good name?

4. +++ b/core/modules/config/src/Tests/ConfigImportUITest.php
   index 3fca8c0..57af7d7 100644
   --- a/core/modules/config/src/Tests/ConfigImporterTest.php
   
   --- a/core/modules/config/src/Tests/ConfigImporterTest.php
   +++ b/core/modules/config/src/Tests/ConfigImporterTest.php
   

   We haven't tested schema mismatch nor version mismatch on themes.

5. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -653,6 +653,17 @@ public static function batchFinished($success, $results, $operations) {
   +    foreach (\Drupal::moduleHandler()->getModuleList() as $module => $extension) {
   

   At least the module handler is injected already into this class.

6. +++ b/core/modules/system/system.install
   @@ -1884,3 +1885,46 @@ function system_update_8014() {
   +  $versions = array_map($map_version_info, \Drupal::moduleHandler()->getModuleList()) +
   

   Can installation profiles have version numbers as well? Oh wait, they are included in the module list, right?

7. +++ b/core/modules/system/system.install
   index f1a1414..b9c633f 100644
   --- a/core/tests/Drupal/KernelTests/KernelTestBase.php
   
   --- a/core/tests/Drupal/KernelTests/KernelTestBase.php
   +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   

   We should apply the updates to \Drupal\KernelTests\KernelTestBase::enableModules as well.

@alexpott, @catch, and I agreed that this is a major bug because of the impact for data integrity.

We might want to consider whether there is a risk of disruption here for some workflows (as @dawehner suggests). The patch requires (and has) an upgrade path, but I'm wondering if we should consider targeting this change for 8.2.x. On the other hand, though, the longer this is not fixed, the more likely it is that sites will get into a bad state with an invalid import, so maybe the risk of this not being fixed is greater. I can see the case for either. If we do target this for 8.1.x I think it definitely merits a CR and release notes mention, even though it is not actually an API change per se, because it could impact production workflows in unexpected ways.

Just asking myself, would this mean that I can't arbitrary import configuration from a site instance to another where even a single one of my module's versions is not the same ? For example, reverting a most recent, in-development instance configuration using the production/stable environment configuration ? Or, let's say I just upgraded a single module but want to run my import once again ? Or I changed some version in my composer.json and run the site install with the upgraded module ?

Actually, I do think this is gonna be so hard to work with CMI that I probably just won't. I mean, you are never expected to synchronize your config by accident.

I would definitely prefer having a "prod" vs "dev" environment (such as Symfony does) and be that cautious only in "prod" environment, but allow the developer to do about everything unsafe he needs to a "dev" environment.

Moreover, in real life, it does happen that you want to do something unsafe on a prod environment as long as you have tested it and know it'll work.

This restriction is way too heavy, it implies that no one will ever have the flexibility of doing unsafe upgrades; as soon as there is no work-around given with this, it gets critical for thousands of developers or dev-ops preventing them doing their upgrades or fixing broken environments with last resort measures.

I'm OK with this happening in D8, but only if I can write --no-strict or --no-check-versions option when running sync to be able to run unsafe upgrades. Users are not that stupid, let them do stupid things when they need to.

Is ignoring patch version safe? That relies on module maintainers tagging versions perfectly. I'd rather be ultra safe, especially if problems don't necessarily show up immediately.

If there was a no-strict option that thrill seekers could use, it would make it difficult for anyone to object to being safe by default.

Just asking myself, would this mean that I can't arbitrary import configuration from a site instance to another where even a single one of my module's versions is not the same ?

That was my immediate thought as well - that would make config imports a quite useless tool for every day project deployments. It seems to me that what we lack here is config schema versioning. We can only decide whether some config is safe to import based upon config schema versions. Best that would follow something like semantic versioning also, so that additions can be done in patch level changes and upgrades in minor level changes. Major version changes in config schema shouldn'T occour, that would be a major version change in the whole module imo.

I agree with fago here. At the very least we need to have an option to use the --force.

I guess one of the points is make sure everybody uses a safe sequence when updating and deploying configuration of a site. And as such the updates have to run ALWAYS first as the point of update hooks is to make sure the database schema is back in sync with the code current version. (And the post_update hooks to fix the content broken in the update hook.) And only then import configuration and then fixing the content broken due to configuration changes. And that is already on a good path with #2762235: Ensure that ConfigImport is taking place without outstanding updates.

We don't at the moment have an easy way to know whether the configuration change was made by a site builder or a schema change (and thus the module maintainer.) Or do we?

On almost 2 years to the day, here's an attempt at re-rolling #27.

Reroll after a year.
Fixed conflicts manually on:

• core/lib/Drupal/Core/Extension/InfoParserDynamic.php
• core/lib/Drupal/Core/Extension/ModuleInstaller.php
• core/modules/system/system.install

From #44:

It seems to me that what we lack here is config schema versioning. We can only decide whether some config is safe to import based upon config schema versions. Best that would follow something like semantic versioning also, so that additions can be done in patch level changes and upgrades in minor level changes. Major version changes in config schema shouldn'T occour, that would be a major version change in the whole module imo.

The process for one of my current projects, when I found it, was to run `drush deploy` in the dev environment (after cloning DB & files from Prod), and then import config. If all went well, we'd do this on Prod. A little while ago, I noticed that upstream config was missing even though the DB schemas were up-to-date.

What I eventually discovered was that new config was generated, but was then being overwritten as discussed in #3285200-17: Importing older config without the mysql/pgsql module enabled will generate error. I then rewrote the process to match the new documentation.

The problem is that you don't get any errors, like in #3285200: Importing older config without the mysql/pgsql module enabled will generate error, but you eventually discover that you're missing config even though your DB schema is up-to-date. Would it be possible to prevent running `drush config:import` if it would be overwriting older config?

To do this, we'd certainly need to version each config file so that when you try to overwrite, the system can stop you from "overwriting the data with an out-of-date backup", but it could be more trouble than it's worth.

What about keeping it simple, and just take the randomness out of site management:

Nothing evil happens if
- after composer require/remove/update, you do db-update AND config-export (the schema might have changed)

If on config-export, we save a copy of composer.json&lock to the config-export, we can add suitable warnings:
- "The requested config-import does not match current codebase. Diff: ..."

One step further: Provide a composer plugin that triggers db-dump, db-update, config-export. This ensures non mismatch in the first place.

Makes sense to me so far. There are scenarios where you don't want/need to export config after an update though, say on Prod if you simply want to enforce your config there (which is usually the ultimate destination). To handle that, maybe we could support for:

drush updatedb --no-config-export

If you just run drush updatedb, you'd get a message like:

Once database schema updates have completed, configuration will be exported so that you can save it. This prevents old configuration, when imported, from overwriting the new configuration generated here. To disable this, rerun this command with the --no-config-export switch instead.

The Composer plug-in is a little too gravy-ish for now. Let's worry about that one later. ;)

Per @catch, this could've prevented #3374544: TypeError: array_intersect(): Argument #1 ($array) must be of type array, string given in array_intersect() (line 203 of .../core/modules/ckeditor5/src/Plugin/CKEditor5PluginManager.php)..