It is possible to validate configuration by checking that all the dependencies exist prior to importing. This will make the configuration import more robust by doing a pre flight check that what is being provided is sane.

How to break your site with HEAD

  1. Site config exported to a git master branch.
  2. Developer A: Deletes the only instance of a field, exports, and commits this git on a branch dev-a
  3. Developer B: Uses this field in on another content type, exports and commits this git on a branch dev-b
  4. Team lead: Merges dev-a and dev-b to master
  5. Developer B: pulls master and runs config import - field storage is deleted (data loss) and field on the other content type is completely broken resulting

This patch resolves this by informing the developer B that the configuration cannot be imported because of missing dependencies.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because through merging configuration changes from multiple developers (which do not conflict) it is possible to create an invalid configuration export that on import will break your site and potentially result in data loss.
Issue priority Critical because once the configuration is imported and data changes made as a result of field changes it is impossible to go back without having having a database backup.
Disruption Not disruptive at all since its only an addition.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Priority: Normal » Major
Status: Needs review » Active
Issue tags: +VDC
Related issues: +#2341357: Views entity area config is not deployable and missing dependencies
alexpott’s picture

Status: Active » Needs review
FileSize
5.23 KB

Here's a (very rough) patch that implements the functionality. I've tested by running an import through xdebug and stepping through. Lets see what breaks.

Status: Needs review » Needs work

The last submitted patch, 2: 2416109.2.patch, failed testing.

alexpott’s picture

I'm not convinced we should be checking dependencies on configuration during sync - at the moment their are too many ways to generate missing dependencies. For example, deleting anything a view depends on does not delete the view. I think I would rather leave this to contrib.

catch’s picture

For example, deleting anything a view depends on does not delete the view.

Shouldn't the View either get deleted, or implement the self-clean-up method, then?

alexpott’s picture

xjm’s picture

I'm not convinced we should be checking dependencies on configuration during sync - at the moment their are too many ways to generate missing dependencies. For example, deleting anything a view depends on does not delete the view. I think I would rather leave this to contrib.

I think we should at least report them?

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.49 KB

Well #2416409: Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted was possible! yay!

Cleaned up the patch a bit. Still needs explicit testing and the think we need to deal with partial imports since we have to deal with #2224571: Single configuration import must run dependencies check before the actual import execution

alexpott’s picture

FileSize
8.04 KB
14 KB

Patch with tests :)

Also @webflo pointed out to me in IRC that we need validate simple config too - which I've added.

alexpott’s picture

FileSize
4.33 KB
14.1 KB

@bircher identified another way that this is useful.

Status: Needs review » Needs work

The last submitted patch, 10: 2416109.10.patch, failed testing.

bircher’s picture

Assigned: Unassigned » bircher
Issue tags: +D8 Accelerate Dev Days

Yes, we need to make sure the whole configuration that is about to be imported is valid.
Otherwise it is too easy to create invalid configuration by merging exported configuration in a multi-developer set up.

bircher’s picture

Status: Needs work » Needs review
FileSize
15.27 KB
1.17 KB

in a first step, fixing the broken test.
The next step I am working on is to make a test that asserts that the configuration can only be imported if it is in itself consistent.

But ideally I would like to re-factor it in such a way that the validation can also be used to validate the self-consistency of a subset of configuration and also just run the validation for a "dry-run" import.

bircher’s picture

Status: Needs review » Needs work

"needs review" was just for the test-bot.

bircher’s picture

Status: Needs work » Needs review
FileSize
16.02 KB
2.63 KB

adding assertion in test.

alexpott’s picture

Issue tags: -Needs tests

We have tests

alexpott’s picture

Issue summary: View changes
FileSize
705 bytes
16.02 KB

Fixing method scope and adding beta evaluation.

Changing to critical for the reasons mentioned in the beta evaluation.

alexpott’s picture

Priority: Major » Critical

As per #17

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -240,11 +240,12 @@ public function getStorageComparer() {
    +    $this->extensionChangelist = $this->processedExtensions = $this->getEmptyExtensionsProcessedList();
    

    Its quite confusing that those two variables are so similar: a list of processed extensions vs. one which is going to be processed, but for sure, this is not the problem of this issue.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +38,79 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +  /**
    +   * Validates all themes being installed during a configuration import exist.
    +   *
    

    Its kinda confusing that we validate both in ConfigImporter and in ConfigImportSubscriber. Would it make sense to mov the module code in a follow up also into the subscriber?

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +38,79 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +   * @param ConfigImporter $config_importer
    ...
    +   * @param ConfigImporter $config_importer
    
    +++ b/core/modules/system/src/SystemConfigSubscriber.php
    @@ -7,32 +7,58 @@
    +   * @param ConfigImporterEvent $event
    

    FQCN

  4. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +38,79 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +    $themes = \Drupal::service('theme_handler')->rebuildThemeData();
    

    Is there a reason we don't use

    $theme_handler->listInfo()/code>
    </li>
    
    <li>
    <code>
    +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +38,79 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +        $config_importer->logError($this->t('Unable to install theme %theme since it does not exist.', array('%theme' => $theme)));
    

    Long term question | OT ... would it make sense to have it available which file is wrong using same parameter in logError?

  5. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +38,79 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +      if (isset($data['dependencies']) && isset($data['uuid'])) {
    

    Can we make a comment why we need to check the UUID here?

  6. +++ b/core/modules/config/src/Tests/ConfigImporterTest.php
    @@ -541,4 +541,61 @@ function testIsInstallable() {
    +    // The whole configuration needs to be  consistent not only the updated one.
    

    2 whitespaces after be ...

  7. +++ b/core/modules/config/src/Tests/ConfigImporterTest.php
    @@ -541,4 +541,61 @@ function testIsInstallable() {
    +      $this->assertFalse(FALSE, 'ConfigImporterException not thrown, invalid import was not stopped due to missing dependencies.');
    

    Don't we use $this->pass() in that case? The code reads like it should be ->fail() though ?

  8. +++ b/core/modules/system/src/SystemConfigSubscriber.php
    @@ -7,32 +7,58 @@
    +   * @param ConfigImporterEvent $event
    

    FQCN

  9. +++ b/core/modules/system/src/SystemConfigSubscriber.php
    @@ -7,32 +7,58 @@
    +    $events[ConfigEvents::IMPORT_VALIDATE][] = array('onConfigImporterValidateNotEmpty', 512);
    +    $events[ConfigEvents::IMPORT_VALIDATE][] = array('onConfigImporterValidateSiteUUID', 256);
    

    I'd consider 256 still a quite high priority.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Thanks for the review @dawehner!

  1. Yeah - not this patch
  2. Yep I'll open a followup to move the rename validation to this event.
  3. Fixed
  4. Yep we should use listInfo() - injected the theme handler
  5. Fixed
  6. Fixed
  7. Fixed - this was a c&p - so fixed the other one.
  8. Fixed
  9. Well I think it is nice when there is lots of space

re

Long term question | OT ... would it make sense to have it available which file is wrong using same parameter in logError?

Yes maybe. Shall I open a followup for this?

dawehner’s picture

Yes maybe. Shall I open a followup for this?

It seems to be worth to discuss, here is a follow up: #2486195: Make config import log errors more machine readable

alexpott’s picture

Status: Needs review » Needs work

The last submitted patch, 22: 2416109-bircher.22.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
950 bytes
17.95 KB

So #19.4 so we need all themes not only installed themes.

dawehner’s picture

jibran’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

A snarky comment also needs a RTBC

alexpott’s picture

Re #28 - nope.

xjm’s picture

Assigned: bircher » xjm

Taking a look at this.

xjm’s picture

xjm’s picture

Assigned: xjm » Unassigned
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
95.59 KB
16.53 KB

Alrighty! I spent a lot of time testing different scenarios.

In HEAD

Note: I discovered #2486475: Notifying user of config changes when config has never been synched still makes no sense in the process of testing this. The yellow warning that appears with the steps below is unrelated to this issue and should be ignored for purposes of testing this.

I was able to reproduce the data-loss-inducing, site-breaking scenario described in the summary with the following steps.

  1. Make a site_config directory and run git init --bare.
  2. Install standard for site A.
  3. Clone the site_config repo into site A's staging directory.
  4. Run drush cex -y on site A, commit, push.
  5. Create a site B from site A (either by copying codebase, files, and db, or by installing using the config_installer project).
  6. Clone the site_config repo into site B's staging directory as well.
  7. On site B, add some article nodes with tags in the tags field.
  8. On site A, delete the tags field at /admin/structure/types/manage/article/fields/node.article.field_tags/delete.
  9. On site A, drush cex -y, git add -u in the staging directory, commit, and push.
  10. On site B, without pulling or anything, add the tags field to the page content type.
  11. On site B, create a couple articles and pages with tags in the tags field.
  12. On site B, run drush cex -y. Add and commit the changes in the staging directory.
  13. On site B, git pull. The changes from site A are automatically merged.
  14. On site B, import the configuration at /admin/config/development/configuration/. Nothing informs me that I'm about to delete data from my page nodes.
  15. Try to visit the "Manage fields" form for the page content type. You will get the following uncaught exception, permanently, rendering the form unusable:
  16. Drupal\Core\Field\FieldException: "Attempt to create a field field_tags that does not exist on entity type node." at /Applications/MAMP/htdocs/config_install_d8git/core/modules/field/src/Entity/FieldConfig.php line 291
  17. On site B, try to visit a page node. You get the same exception and no content is displayed.

You broke all your page nodes in a way that is unrecoverable, congrats! So yeah, this is a critical.

With the patch

  1. I applied the patch and then tried the same steps. as above Now, the configuration import is properly prevented with the message:

    Configuration field.field.node.page.field_tags depends on configuration that will not be installed after import.

    I confirmed that the import is not started at all, that the page nodes and admin form still work, and that no data is lost. The one issue I noticed here is that the messages are now pretty confusing.

    I love that "The configuration synchronization failed validation" is a happy green message. You failed validation -- thumbs up! Per @alexpott this is not introduced here, but the patch will make it so that users see this message a lot more frequently, so we should change it to be properly an error.

    The new message also doesn't really tell the user what this means nor how to deal with it, so I think we could do some work to improve that as well. When I first read "will not exist after import", at first I thought "So what? Isn't that the point of doing an import?" (I'm assuming the message is this way now because the missing dependency could either be a difference between the current site and the staged config, or an internal inconsistency within the staged config, and we can't really tell which it is.)
     

  2. I also tested a view with a dependency on a content entity that was missing. As per https://www.drupal.org/node/2341357 #103, the validation does not fail when a content entity dependency is missing, which is by design. It did seem unexpected though that we give the user no notification whatsoever that there's a missing content dependency, since missing config dependencies stop the import in its tracks. @alexpott suggested fixing this after #2361423: Add a step to config import to allow contrib to create content based on config dependencies (so can we add a followup postponed on that issue?).
     
  3. I then the same view with a missing config dependency instead of a missing content dependency, and the import was prevented correctly, as above.
     
  4. I tried to import the same view with the missing dependency via the single import form. Unlike the config synch, the single import form blindly updates the view but silently removes the config dependency key for the missing configuration entity, even though the view actually still depends on it elsewhere in the configuration. @alexpott filed #2486467: Single config import form needs to use the config validation events for that.
     
  5. I tried using the single configuration import to import a field instance without its storage (to see if I could use this form to break the site similarly to the main scenario above). Turns out the Entity Field API actually prevents the single import from working with the same exception above as an error on that form. Since it prevents the import and no other data is changed, this means that the fact that the single import form ignores validation doesn't cause the thorough brokenness described in the scenario for HEAD above, so I think #2486467: Single config import form needs to use the config validation events is major (not critical).
     
  6. I configured some color settings with Bartik, then try to import an uninstallation of color with no other changes: The import is prevented correctly with:

    Configuration color.theme.bartik depends on the color extension that will not be installed after import.

     

  7. I tried deleting the tags field from the site (so that there was no configuration that depended on Entity Reference), then introducing a "bad merge" into core.extension.yml (actually I just hacked it) that uninstalled entity reference but not taxonomy. This time the import proceeded, resulting in the site being in a buggy state where Taxonomy was installed, but not the Entity Reference module it depends on. The import validation should be respecting module dependencies too.
     

I think that #1 and #7 should be fixed in this issue, so setting NW for those. (I haven't done a code review yet.)

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.23 KB
22.26 KB

Patch attached fixes 1 and 7 from #33. The patch also adds validation to ensure that a module which does not exist in the incoming core.extension file. Plus theme dependencies are also checked. The validations are tested as well.

jibran’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
@@ -58,27 +58,61 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
+  protected function validateModules($config_importer) {

@param type missing :)

Status: Needs review » Needs work

The last submitted patch, 34: 2416109-bircher.34.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.42 KB
24.56 KB

Fixed the test and #35.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -58,27 +58,61 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
         if ($config_importer->getStorageComparer()->getSourceStorage()->exists('core.extension')) {
    ...
         }
       }
    

    Should we validate that this file actually exists? No idea how hell freezed that this happened but checking for it, seems reasonable.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -58,27 +58,61 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    -  protected function validateThemeInstalls(ConfigImporter $config_importer) {
    +  protected function validateModules($config_importer) {
    

    Do you mind adding a typehint?

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -58,27 +58,61 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +    $uninstalls = $config_importer->getExtensionChangelist('theme', 'uninstall');
    +    foreach ($uninstalls as $theme) {
    

    Is it worth to extract that duplication into its own protected method, so its not required to do things twice?

  4. +++ b/core/modules/config/src/Form/ConfigSync.php
    @@ -340,7 +340,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -        drupal_set_message($this->t('The configuration synchronization failed validation.'));
    +        drupal_set_message($this->t('The configuration synchronization failed validation.'), 'error');
    

    Nice fix!

alexpott’s picture

  1. Sure
  2. Huh - I thought I added them all - couldn't find the missing typehint
  3. Nope because the message is different - theme vs modules
  4. Thanks

Also tried to address the error message issues described by #33.1. See image below:

dawehner’s picture

Looks perfect for me now, but I guess we need some additional feedback from xjm here?

catch’s picture

Very minor nit but:

+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
@@ -37,6 +56,122 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
+    $nonexistent_modules = array_diff(array_keys($core_extension['module']), array_keys($module_data));

array_keys(array_diff_key(..))?

Nothing else to complain about.

alexpott’s picture

FileSize
1016 bytes
25.54 KB

Nice catch @catch - that is totally an @alexpott-ism :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Awesome!

xjm’s picture

Status: Reviewed & tested by the community » Needs work

@alexpott is working on some additional improvements on the messages per our discussion.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
175.82 KB
11.76 KB
30.63 KB

Display which modules, themes and modules are missing when configuration dependencies validation fails.

dawehner’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
@@ -156,22 +195,86 @@ protected function validateDependencies(ConfigImporter $config_importer) {
+   * Gets theme data.
+   *
+   * @return \Drupal\Core\Extension\Extension[]
+   */
+  protected function getThemeData() {
+    if (!isset($this->themeData)) {
+      $this->themeData = $this->themeHandler->rebuildThemeData();
+    }
+    return $this->themeData;
+  }
 
+  /**
+   * Gets module data.
+   *
+   * @return \Drupal\Core\Extension\Extension[]
+   */
+  protected function getModuleData() {
+    if (!isset($this->moduleData)) {
+      $this->moduleData = system_rebuild_module_data();
+    }
+    return $this->moduleData;
+  }

You see the structure ... of why we need a extension list mechanism :P

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Anyway, one nasty comment, and night and one RTBC

catch’s picture

Assigned: Unassigned » xjm

Giving xjm a last chance to review this one, agreed those messages are more useful.

alexpott’s picture

@xjm asked me about performance and this patch. Reading all the configuration from disk would be extremely expensive. However, since #2411689: Use a MemoryBackend in StorageComparer so that configuration import validators don't have to reread data from disk or the db we statically cache all config read during validation so reading config is essentially free.

xjm’s picture

Assigned: xjm » alexpott
Status: Reviewed & tested by the community » Needs work

Reviewed and discussed lots of things with alexpott in person, some of which need changes. Apologies for the terseness of the following notes; some are comments to myself. :P

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -240,11 +240,12 @@ public function getStorageComparer() {
    -    $this->createExtensionChangelist();
    

    Hm. Look at why this method was called before and why it no longer is.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -404,22 +408,19 @@ protected function createExtensionChangelist() {
    +    $this->extensionChangelist['module']['install'] = array_intersect(array_keys($module_list), $install);
    +    if ($missing_modules = array_diff($install, $this->extensionChangelist['module']['install'])) {
    +      $this->logError($this->formatPlural(
    +        count($missing_modules),
    +        'Unable to install module %modules since it does not exist.',
    +        'Unable to install modules %modules since they do not exist.',
    +        array('%modules' => implode(', ',$missing_modules))
    +      ));
    +    }
    

    Why no equivalent with themes? And why does this exist again in validateDependencies() when other hunks like it don't? Discussed with alexpott and it actually looks like this hunk can be removed now.

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -18,6 +20,37 @@
    +  public function __construct(ThemeHandlerInterface $theme_handler) {
    +    $this->themeHandler = $theme_handler;
    +  }
    

    Why is it we need to inject the theme handler but not the module handler?

  4. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +    else {
    +      $config_importer->logError($this->t('The core.extension configuration does not exist.'));
    +    }
    

    Nice to see this fixed in here; hopefully I'll see a test for this later on. Edit: per alexpott it isn't tested yet. :)

  5. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +  protected function validateModules(ConfigImporter $config_importer) {
    ...
    +  protected function validateThemes(ConfigImporter $config_importer) {
    

    Why is there nothing in this module about missing dependencies being not installed? "Unable to install Taxonomy since ER is not installed" or such. Ditto for themes. Edit: alexpott confirmed that this is broken and will post an updated patch fixing it.

  6. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +    foreach ($nonexistent_modules as $module) {
    +      $config_importer->logError($this->t('Unable to install module %module since it does not exist.', array('%module' => $module)));
    +    }
    

    Why is this redundant with the other hunk above in ConfigImporter::createExtensionChangelis()?

  7. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +    // Ensure that module dependencies are respected.
    +    $uninstalls = $config_importer->getExtensionChangelist('module', 'uninstall');
    +    foreach ($uninstalls as $module) {
    +      foreach (array_keys($module_data[$module]->required_by) as $dependent_module) {
    +        if ($module_data[$dependent_module]->status && !in_array($dependent_module, $uninstalls, TRUE)) {
    

    It's interesting that the extension system doesn't provide API for checking this, but maybe that's part of what @dawehner is snarking about. (That said, +1 for doing it here with the current codebase.)

  8. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +          $config_importer->logError($this->t('Unable to uninstall module %module since %dependent_module is installed.', array('%module' => $module_name, '%dependent_module' => $dependent_module_name)));
    

    I would say "is still installed" maybe? Not important. Maybe not.

  9. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +    $existing_dependencies = [
    

    Why is this "existing" dependencies? The source is the staged config, no?

  10. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +    foreach ($config_importer->getStorageComparer()->getSourceStorage()->listAll() as $name) {
    ...
    +      $data = $config_importer->getStorageComparer()->getSourceStorage()->read($name);
    

    Wait how is the second stuff different? @alexpott explained this to me: First it evaluates implicit dependencies on the module that owns the config entity type. Then it evaluates explicit dependencies declared in the entity afterward. I'll add a couple proposed comments.

  11. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +      list($owner,) = explode('.', $name, 2);
    

    Don't we have an API method for this yet?

  12. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +      if ($owner !== 'core') {
    

    So obviously, we don't need to check this for core because core is always installed. Cool.

  13. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +      // Configuration entities can be identified by having a dependencies and a
    +      // UUID key. Check the dependencies to ensure they are met.
    

    Minor: By having 'dependencies' and 'uuid' keys?

  14. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -37,6 +70,211 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +   * Gets human readable extension names.
    ...
    +   *   A list of human readable extension names or machine names if not
    +   *   available.
    

    Nit: human-readable. But also, aren't they just "labels" or something? Or whatever the key is in info files.

  15. +++ b/core/modules/config/src/Form/ConfigSync.php
    @@ -340,7 +340,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -        drupal_set_message($this->t('The configuration synchronization failed validation.'));
    +        drupal_set_message($this->t('The staged configuration cannot be imported because it failed validation for the following reasons:'), 'error');
    

    Minor/maybe out of scope: If we're talking about moving language away from "staging" (because of the potential confusion with a specific staging environment), should we change the wording here to not refer to "staged configuration"? Or are we already using that language elsewhere? Add the related issue.

  16. +++ b/core/modules/config/src/Form/ConfigSync.php
    @@ -340,7 +340,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +        drupal_set_message($this->t('The staged configuration cannot be imported because it failed validation for the following reasons:'), 'error');
    
    +++ b/core/modules/config/src/Tests/ConfigImportUITest.php
    @@ -326,7 +326,7 @@ public function testImportValidation() {
    +    $this->assertText('The staged configuration cannot be imported because it failed validation for the following reasons:');
    

    Note to self: Read this test.

  17. +++ b/core/modules/config/src/Tests/ConfigImportUITest.php
    @@ -453,4 +453,35 @@ public function testEntityBundleDelete() {
    +   * Tests config importer cannot uninstall extensions which are depended on.
    

    The wording is a bit weird, but also I can't come up with better.

  18. +++ b/core/modules/config/src/Tests/ConfigImportUITest.php
    @@ -453,4 +453,35 @@ public function testEntityBundleDelete() {
    +    // Node depends on text.
    +    unset($core['module']['text']);
    +    // Bartik depends on classy.
    +    unset($core['theme']['classy']);
    

    Can we add assertions that will fail if node no longer depends on text, or bartik no longer depends on classy?

  19. +++ b/core/modules/config/src/Tests/ConfigImporterTest.php
    @@ -105,7 +105,7 @@ function testSiteUuidValidate() {
    -      $this->assertFalse(FALSE, 'ConfigImporterException not thrown, invalid import was not stopped due to mis-matching site UUID.');
    +      $this->fail('ConfigImporterException not thrown, invalid import was not stopped due to mis-matching site UUID.');
    

    So this is ever so slightly out of scope, but sure. ;)

  20. +++ b/core/modules/config/src/Tests/ConfigImporterTest.php
    @@ -541,4 +541,87 @@ function testIsInstallable() {
    +   * @see \Drupal\Core\EventSubscriber\ConfigImportSubscriber
    +   * @see \Drupal\Core\Config\ConfigImporter::createExtensionChangelist()
    

    Thank you for adding these references, +1.

  21. +++ b/core/modules/field/src/Tests/FieldImportDeleteUninstallUiTest.php
    @@ -23,7 +23,7 @@ class FieldImportDeleteUninstallUiTest extends FieldTestBase {
    -  public static $modules = array('entity_test', 'telephone', 'config', 'filter', 'text');
    

    Why are the changes in this test needed? Per alexpott, this was actually a bug in this test in HEAD, because it was uninstalling text without uninstalling entity_test, which shouldn't actually be possible, and so hey the code works. The point of the test is to examine what happens to entities during an uninstallation of a field-providing module.

  22. +++ b/core/modules/system/src/SystemConfigSubscriber.php
    @@ -7,32 +7,58 @@
    +   * This event listener prevents deleting all configuration. If there is
    +   * nothing to import then event propagation is stopped because there is no
    +   * config import to validate.
    ...
    +  public function onConfigImporterValidateNotEmpty(ConfigImporterEvent $event) {
    ...
    +  public function onConfigImporterValidateSiteUUID(ConfigImporterEvent $event) {
    

    Discussed with @alexpott. The reason that this needs to be separated out into two event subscribers now is that if there's no config to import, the remaining step would blow up now that we are validating. Maybe worth adding test coverage? Or at least the bit that would break if this were broken, which per alexpott is the core.extension check in the import subscriber.

I did not review testUnmetDependency() yet.

alexpott’s picture

Status: Needs work » Needs review
FileSize
14.87 KB
34.19 KB

Thanks @xjm for the great review.

  1. This was replaced with $this->extensionChangelist = $this->processedExtensions = $this->getEmptyExtensionsProcessedList();
  2. There is an equivalent for themes in ConfigImportSubscriber - in fact this is totally unnecessary - all of this has been moved to ConfigImportSubscriber
  3. Because the module handler is not responsible for module data - system_rebuild_module_data() is :(
  4. Added a test.
  5. Fixed and added tests - great catch
  6. Yep I removed the hunk from ConfigImporter
  7. Indeed
  8. Oops missed out this one
  9. Well these are the all the things that will exist after the configuration has been imported - I don't really like other names like $dependencies_that_will_exist - obviously open to suggestions though
  10. Improved comments
  11. Nope
  12. Yep
  13. Fixed
  14. Fixed - the yaml key is name but this clashes with Extension::getName() :)
  15. Fixed - removed the word staged
  16. ok
  17. ok
  18. Fixed added assertions
  19. ok
  20. You're welcome
  21. Yep that's why
  22. There is the ConfigImporterTest::testEmptyImportFails() - can't think of how to test the stopped propagation without added quite a bit of code. It doesn't blow up without this because we no longer assume that core.extension exists but I think it makes sense to stop validation in this case so I'd like to now change this.
alexpott’s picture

re #50.8 maybe a review of all the messages possible would be good I'll post a summary of all them.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

GIve it another read, even I'm still not confident that I have enough knowledge about it, I trust xjm and alexpott.

xjm’s picture

Assigned: alexpott » Unassigned
Status: Reviewed & tested by the community » Fixed

This looks great and seems pretty complete now, has been tested thoroughly manually, and now has nice complete test coverage. It addresses a critical issue and is allowed per https://www.drupal.org/core/beta-changes.

This change is going to make using CMI a lot more robust overall and I think any additional things can be addressed in followups (including #2224571: Single configuration import must run dependencies check before the actual import execution and anything else we might find). Committed and pushed to 8.0.x. Yay!

+++ b/core/modules/config/src/Tests/ConfigImporterTest.php
@@ -571,10 +571,14 @@
+    // Add a module and a theme that do depend on uninstalled extensions.

I do declare! :) I made some very small wording and punctuation fixes here and elsewhere on commit:

diff --git a/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
index 507da95..28af01b 100644
--- a/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
@@ -144,7 +144,7 @@ protected function validateThemes(ConfigImporter $config_importer) {
     foreach ($installs as $key => $theme) {
       if (!isset($theme_data[$theme])) {
         $config_importer->logError($this->t('Unable to install the %theme theme since it does not exist.', array('%theme' => $theme)));
-        // Remove non existing installs from list so we can validate theme
+        // Remove non-existing installs from the list so we can validate theme
         // dependencies later.
         unset($installs[$key]);
       }
@@ -227,11 +227,11 @@ protected function validateDependencies(ConfigImporter $config_importer) {
       }
 
       $data = $config_importer->getStorageComparer()->getSourceStorage()->read($name);
-      // Configuration entities have dependencies on modules, themes and other
+      // Configuration entities have dependencies on modules, themes, and other
       // configuration entities that we can validate. Their content dependencies
       // are not validated since we assume that they are soft dependencies.
-      // Configuration entities can be identified by having dependencies and
-      // UUID keys.
+      // Configuration entities can be identified by having 'dependencies' and
+      // 'uuid' keys.
       if (isset($data['dependencies']) && isset($data['uuid'])) {
         $dependencies_to_check = array_intersect_key($data['dependencies'], array_flip(['module', 'theme', 'config']));
         foreach ($dependencies_to_check as $type => $dependencies) {
@@ -307,8 +307,8 @@ protected function getModuleData() {
    *   Extension data.
    *
    * @return array
-   *   A list of human-readable extension names or machine names if not
-   *   available.
+   *   A list of human-readable extension names, or machine names if
+   *   human-readable names are not available.
    */
   protected function getNames(array $names, array $extension_data) {
     return array_map(function ($name) use ($extension_data) {
diff --git a/core/modules/config/src/Tests/ConfigImporterTest.php b/core/modules/config/src/Tests/ConfigImporterTest.php
index 6f8c10c..c830b45 100644
--- a/core/modules/config/src/Tests/ConfigImporterTest.php
+++ b/core/modules/config/src/Tests/ConfigImporterTest.php
@@ -564,7 +564,7 @@ public function testUnmetDependency() {
     $staging->write('config_test.dynamic.dotted.config', $config_entity_data);
 
     // Make an active config depend on something that is missing in staging.
-    // The whole configuration needs to be consistent not only the updated one.
+    // The whole configuration needs to be consistent, not only the updated one.
     $config_entity_data['dependencies'] = [];
     $storage->write('config_test.dynamic.dotted.deleted', $config_entity_data);
     $config_entity_data['dependencies'] = ['config' => ['config_test.dynamic.dotted.deleted']];
@@ -575,14 +575,14 @@ public function testUnmetDependency() {
     // Add a module and a theme that do not exist.
     $extensions['module']['unknown_module'] = 0;
     $extensions['theme']['unknown_theme'] = 0;
-    // Add a module and a theme that do depend on uninstalled extensions.
+    // Add a module and a theme that depend on uninstalled extensions.
     $extensions['module']['book'] = 0;
     $extensions['theme']['bartik'] = 0;
 
     $staging->write('core.extension', $extensions);
     try {
       $this->configImporter->reset()->import();
-      $this->fail('ConfigImporterException not thrown, invalid import was not stopped due to missing dependencies.');
+      $this->fail('ConfigImporterException not thrown; an invalid import was not stopped due to missing dependencies.');
     }
     catch (ConfigImporterException $e) {
       $this->assertEqual($e->getMessage(), 'There were errors validating the config synchronization.');

  • xjm committed 8f5c324 on 8.0.x
    Issue #2416109 by alexpott, bircher, xjm, dawehner: Validate...

Status: Fixed » Closed (fixed)

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