Problem/Motivation

It is important that we people sync configuration is done against the same codebase in both the target and source instance. People have been surprised that you can't sync configuration from Beta-13 to RC-3. This issue will get worse with contrib under active development. We also need to ensure that extensions are installed using the same version of the extension so that any install hooks are the same and we need this to be able to solve #2428045: Comparing active storage to install storage is problematic, install storage may change anytime without storing a copy of the original config in config - this way we can store it in state.

Proposed resolution

When we run update.php write the current versions of the extensions to core.extension. When we install an extension or run updates write the version to core.extension. When we update or install a module store the schema version in core.extension. Use this information to ensure that when we import a full site configuration the codebase and database are in the same state.

Remaining tasks

Review all of the UI text

User interface changes

Additional configuration import validation messages

API changes

None

Data model changes

Additional versions key added to core.extensions. The key holds a list of installed extensions. For each extension the version at install time is stored plus the version the last time update.php is run. For modules, the schema is also stored.

Files: 
CommentFileSizeAuthor
#27 2628144-27.patch33.19 KBalexpott
#27 25-27-interdiff.txt1.09 KBalexpott
#25 2628144-25.patch33.18 KBalexpott
#25 21-25-interdiff.txt13.42 KBalexpott
#21 2628144-21.patch34.07 KBalexpott
#21 19-21-interdiff.txt2.31 KBalexpott
#19 2628144-19.patch34.07 KBalexpott
#19 18-19-interdiff.txt1.92 KBalexpott
#18 2628144-18.patch32.61 KBalexpott
#18 14-18-interdiff.txt6.63 KBalexpott
#14 2628144-14.patch29.38 KBalexpott
#14 12-14-interdiff.txt4.04 KBalexpott
#14 2628144-14.patch29.38 KBalexpott
#14 12-14-interdiff.txt4.04 KBalexpott
#12 2628144-12.patch26.89 KBalexpott
#12 10-12-interdiff.txt1.75 KBalexpott
#10 2628144-10.patch26.85 KBalexpott
#10 7-10-interdiff.txt1.31 KBalexpott
#7 2628144-7.patch26.97 KBalexpott
#7 6-7-interdiff.txt9.91 KBalexpott
#6 2628144-6.patch19.79 KBalexpott
#6 4-6-interdiff.txt1.99 KBalexpott
#4 2-4-interdiff.txt20.56 KBalexpott
#4 2628144-4.patch19.77 KBalexpott
#2 2628144-2.patch8.7 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
8.7 KB

So here's recording the installation time versions... need to work out how to record the module version at export time. I think maybe we just need to record this as part of update.php.

Status: Needs review » Needs work

The last submitted patch, 2: 2628144-2.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
19.77 KB
20.56 KB

Still need to add current version and schema checking to the validator... but now have got the info in core.extension and have an upgrade path - that needs testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2628144-4.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
19.79 KB

Schema fun...

alexpott’s picture

Add tests and validating that both the source and target site instances have the same versions of modules and that the installed schema version is the same.

alexpott’s picture

Status: Needs review » Needs work

The last submitted patch, 7: 2628144-7.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
26.85 KB

I love config schema - keeps me honest :)

Status: Needs review » Needs work

The last submitted patch, 10: 2628144-10.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
26.89 KB

Fixing the tests...

swentel’s picture

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.

alexpott’s picture

Thanks for the review @swentel.

  1. Fixed
  2. Fixed
  3. Yeah this is a tricky thing... maybe inline with the experimental modules changes this could be 8.y.x-unknown as we know the major version. Perhaps this should be split off to a precursor to discuss but none is also true.
  4. Yes we should test this... added one
  5. Yes we should test this... added one
  6. There are two KernelTestBaseTest one based on SimpleTest and one on PHPUnit.
alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
+++ b/core/modules/system/system.install
@@ -1871,3 +1871,29 @@ function system_update_8014() {
+function system_update_8015() {

Needs tests

The last submitted patch, 14: 2628144-14.patch, failed testing.

The last submitted patch, 14: 2628144-14.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.63 KB
32.61 KB

Here an upgrade path test.

I also think we should have explicit tests for the update system changes so leaving the "Needs tests" tag on .

alexpott’s picture

Here's a test for the changes to drupal_set_installed_schema_version()

Status: Needs review » Needs work

The last submitted patch, 19: 2628144-19.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.31 KB
34.07 KB

Hmmm oh yeah schema needs to be a string... because that is the documented return value of...

 * @return string|int
 *   The currently installed schema version, or SCHEMA_UNINSTALLED if the
 *   module is not installed.
 */
function drupal_get_installed_schema_version($module, $reset = FALSE, $array = FALSE) {

Status: Needs review » Needs work

The last submitted patch, 21: 2628144-21.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

Drupal\aggregator\Tests\AddFeedTest passes fine locally... drupalci?

alexpott’s picture

Status: Needs review » Needs work

I've been think about the install information. I think this might be a bit restrictive. Consider creating a site from config - it could make it impossible as the install and current versions don't match. So I think this should be removed.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
13.42 KB
33.18 KB

Removed the installed information.

Status: Needs review » Needs work

The last submitted patch, 25: 2628144-25.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
33.19 KB

Fixing the test fail and making sure the update works as expected.

swentel’s picture

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 ?

davidwbarratt’s picture

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?

alexpott’s picture

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

This would be doing it wrong - if a module update creates an entity of a bundle that it itself does not provide it has to handle what would occur of those dependencies are missing. This is (imo) totally separate from the issue of importing config.

dawehner’s picture

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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: +Triaged D8 major

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

xjm’s picture

Issue tags: +Needs change record

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.

pounard’s picture

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 ?

alexpott’s picture

@pounard well it would make these situations more tricky - but reverting to previous config that ran against a different code base is inherently dangerous and exactly what we want to prevent.

alexpott’s picture

One thing we could consider is ignoring the patch version to make it looser.

pounard’s picture

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.

alexpott’s picture

@pounard but wouldn't ignoring patch differences work + a flag to make dev's lives simple whilst protecting everyone from breaking their sites because the problem with

Users are not that stupid, let them do stupid things when they need to.

Is that if a site crashes to a white screen it is not so much letting them - it's trying to prevent irrecoverable mistakes.

jonathanshaw’s picture

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.

alexpott’s picture

Created #2762235: Ensure that ConfigImport is taking place without outstanding updates to do something smaller and easier to agree on.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fago’s picture

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.

bircher’s picture

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?