Updated: Comment #30

Invalid Config can get imported and hose your site

Proposed resolution

Implement validate hooks

Remaining tasks

  1. #1862160: config() "on not found" behaviour flag is necessary
  2. One use case follows on from #1735118: Convert Field API to CMI. If a config import proposes to change a Field's type, an FieldException gets thrown by field_update_field. It would be good if this exception were handled during validation, and not after the import has started.
  3. Incoming views should be validated

API changes

None

Original report by @moshe weitzman

Comments

sun’s picture

Issue tags: +Configuration system

Thanks for writing down a use-case. That makes sense.

Is it safe to make the following assumptions?

  1. Hook implementations need access to the configuration data being imported.
  2. Hook implementations additionally need access to the previously existing configuration data (if any).
  3. If a hook implementation rejects a change, the import should not even start. (?)

    In other words: Not individual config objects are rejected/skipped, but the entire import operation, because otherwise, all possible interdependencies would have to be figured out (and all other affected/dependent config objects would have to be rejected, too).

These assumptions would have a range of implications on the config import process (mainly surrounding performance). Therefore, let's make sure to hash out the exact use-case and requirements before jumping into code.

moshe weitzman’s picture

Yes, I agree with all of those assumptions.

beejeebus’s picture

yay for this issue. i agree with 1. it's exactly how the import patch at drupalcon denver worked. the one i discussed at length with sun at the time.

would love to see that come back to life. i didn't want the current import code to land without validation, but i was absent from CMI dev at the time because it was just toxic.

sun’s picture

Ok, I understand the use-case and why we need it.

I am primarily concerned about 1) memory consumption and 2) performance.

Giving all extensions a chance to potentially say No to certain changes, before any import/change is done, means that we have to load all configuration objects (plus the original/existing) into memory a first (second) time, so as to be able to hand them off for validation.

Currently, these aspects are hard to measure, since we still only have a few config objects (I'd expect roundabout ~500 on production sites), and secondly, because we did not implement static caching yet.

These two aspects should be kept in mind, but otherwise, this should be relatively simple to implement.

moshe weitzman’s picture

@sun - if it is easy for implementers of this hook to load only the existing config that they need to inspect, then lets do that. We don't need to proactively do that. Hopefully that saves memory.

sun’s picture

That's an interesting idea — I'm not sure whether this counts as easy or complex:

        $old_config = new Config($name, $target_storage);
        $old_config->load();

        $data = $source_storage->read($name);
        $new_config = new Config($name, $target_storage);
        if ($data !== FALSE) {
          $new_config->setData($data);
        }

That's what config_import_invoke_owner() is doing before it invokes the config_import_*() hooks.

So if we did not pass fully loaded config objects and leave the loading to validation hook implementations, then I guess a potential implementation would look similar to this?

function field_config_import_validate(array $changes, $source_storage, $target_storage) {
  // Hm. Filter all 'changes' to check for any field changes.
  $field_changes = preg_grep('/^field\.field\./', $changes['change']);

  foreach ($field_changes as $name) {
    // @todo Provide a simpler way to do this.
    $old_config = new Config($name, $target_storage);
    $old_config->load();

    $data = $source_storage->read($name);
    $new_config = new Config($name, $target_storage);
    if ($data !== FALSE) {
      $new_config->setData($data);
    }

    // A field type cannot be changed.
    if ($old_config->get('type') !== $new_config->get('type')) {
      throw new ConfigImportException(format_string('Field type of %field_name cannot be changed.', array('%field_name' => $new_config->get('field_name'))));
    }
  }
}

Yeah, it's probably a good idea to design this from a hook implementator perspective.

heyrocker’s picture

I am a little concerned about the increased amount of time this could add to the import process. In theory people should be putting their sites in maintenance mode during an import, and we need to keep that time space as short as possible. I don't think it will end up being a big concern but its something we should be conscious of.

moshe weitzman’s picture

Anyone up for writing an initial patch here?

beejeebus’s picture

just adding #1862160: config() "on not found" behaviour flag is necessary via chx - it's a good example of what user module should do in it's validate hook to stop a broken site from an invalid import.

working on a first cut patch.

beejeebus’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
7.14 KB
FAILED: [[SimpleTest]]: [MySQL] 49,605 pass(es), 3 fail(s), and 0 exception(s). View

and here we go.

- missing tests
- no idea how to validate views display plugins...

Status: Needs review » Needs work

The last submitted patch, 1842956-config-validate-10.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
7.17 KB
PASSED: [[SimpleTest]]: [MySQL] 49,632 pass(es). View

- fixed missing FileStorage in system.module

damiankloip’s picture

FileSize
7.38 KB
PASSED: [[SimpleTest]]: [MySQL] 49,665 pass(es). View
1.44 KB

I spoke to beejeebus about this briefly on IRC, we could just check that the display plugin exists, because things will blow up if that isn't found. Fields are ok, a view is still editable/viewable etc... in that case.

damiankloip’s picture

FileSize
7.58 KB
PASSED: [[SimpleTest]]: [MySQL] 49,964 pass(es). View
1.25 KB

I guess we should also maybe test that we can create an instance too? As you could have a definition cached but no plugin or some other thing.

dawehner’s picture

+++ b/core/modules/views/views.moduleundefined
@@ -479,6 +480,35 @@ function views_menu_alter(&$callbacks) {
+        throw new ConfigImportInvalidChangeException('View display plugin cannot be found.');

I would personally vote to remove this part, because a view really can run without having the display available. There are tons of checks in views itself which takes care of checking whether the display actually exists. Just imagine a view with a page and a block. If you disable the block module, shouldn't the actual page still work, it at least did in D7.

damiankloip’s picture

The main trouble is that when you go to edit a view in the UI or something, you will get a nasty plugin error, as it tries to load the plugin id but can't find the class anymore :/

dawehner’s picture

Then this is a bug, we really should check that.

sun’s picture

+++ b/core/includes/config.inc
@@ -195,6 +195,47 @@ function config_sync_changes(array $config_changes, StorageInterface $source_sto
+      module_invoke($module, $hook, $changes, $source_storage, $target_storage);

Wouldn't it make sense to pass the entire $config_changes as an additional argument to each hook implementation, so they are able to validate changes that may depend on other changes, too?

For example, Comment module would then be able to validate that the node type, for which comment settings are imported, actually exists or will be created.

+++ b/core/includes/config.inc
@@ -211,6 +252,15 @@ function config_import() {
+  catch (ConfigException $e) {

1) Shouldn't we catch a ConfigImportInvalidChangeException only? Any other exception would be unexpected, no?

2) Shouldn't we make this part of the lock-acquired processing code? Otherwise, the (potentially long running) validation would be able to run more than one in parallel and throw false errors?

+++ b/core/lib/Drupal/Core/Config/ConfigImportInvalidChangeException.php
@@ -0,0 +1,16 @@
+use RuntimeException;

Unused.

+++ b/core/modules/system/system.module
@@ -1056,6 +1058,20 @@ function system_menu() {
+function system_config_import_validate($changes, $source_storage, $target_storage) {
+  // Don't allow any base config files to be deleted by an import.
...
+      throw new ConfigImportInvalidChangeException("Deleting default system config is invalid: '$name'.");

Hm. While this makes some sense, I wonder whether "A list of config object names that I require to operate" isn't something that many more modules aside from System module might want and would have to re-implement by duplicating exactly this code, just with a different directory?

In other words, could we turn this list into simple meta data? Supplied by a new hook_config_required()? Or possibly even defined in the .info file of an extension?

Or, to perhaps think outside of the box, aren't all "static" config objects normally required to exist?

  Default config in ./config
- [config_prefixes]\..+
= Required static config

In other words, couldn't we even automate this by listing all default config of a module and subtracting dynamic default config entity objects, which would result in the list of static/settings objects?

+++ b/core/modules/views/views.module
@@ -479,6 +480,35 @@ function views_menu_alter(&$callbacks) {
+    if (!$view->id()) {
+      throw new ConfigImportInvalidChangeException('View has no id.');
+    }
+    if (!$displays = $view->get('display')) {
+      throw new ConfigImportInvalidChangeException('View has no displays.');
+    }

Hm. I'm not sure whether this is the right layer for validating such essential properties and whether this kind of validation doesn't add a wrong sense of safety.

A view config entity without an ID or display should throw a validation error upon trying to save already — if you manage to save it and even stage it, then you surely must have hacked the system in unsupported ways. ;)

+++ b/core/modules/views/views.module
@@ -479,6 +480,35 @@ function views_menu_alter(&$callbacks) {
+    foreach ($displays as $display) {
+      if (!isset($display_definitions[$display['display_plugin']]) || !is_object($display_manager->createInstance($display['display_plugin']))) {
+        throw new ConfigImportInvalidChangeException('View display plugin cannot be found.');
+      }
+    }

AFAICS, this is the most interesting part of this patch: Validating references.

In this case, config-plugin references.

But the more stuff we convert to config, the more we're seeing config-config, but also config-content references.

One could even argue that the system.site:page.404 & Co settings are config-route references that could use validation.

Any of these reference validations will ultimately lead to situations in which the referenced item may not exist before it has been staged/imported. I'm relatively sure that we will face validation dependency scenarios that will require a CONFIG_DEFER equivalent for the validation step, and which will potentially force us to re-run validation before invoking each owner.

With regard to this concrete code here, I wonder why we have to instantiate an actual instance of the display_plugin, instead of checking whether it can or could be instantiated only? (by checking the plugin registry for the referenced plugin ID)

beejeebus’s picture

Wouldn't it make sense to pass the entire $config_changes as an additional argument to each hook implementation, so they are able to validate changes that may depend on other changes, too?

that seems fair, i think i was trying to pass as a little around as possible.

1) Shouldn't we catch a ConfigImportInvalidChangeException only? Any other exception would be unexpected, no?

i'd be ok with that, but then i think we should wrap the config_import() call in config.admin.inc in a try catch. in the end i don't care too much though, as the import will still be stopped before Bad Things are written to disk, so that's ok.

Hm. While this makes some sense, I wonder whether "A list of config object names that I require to operate" isn't something that many more modules aside from System module might want and would have to re-implement by duplicating exactly this code, just with a different directory?

yep, i considered this as well, and i think it makes sense to do at some point. i left it out purely from the 'will that slow down this patch landing or not?' point of view. so, to anyone else reviewing this - should we put a general solution for that in this patch, or a follow up?

Hm. I'm not sure whether this is the right layer for validating such essential properties and whether this kind of validation doesn't add a wrong sense of safety.

A view config entity without an ID or display should throw a validation error upon trying to save already — if you manage to save it and even stage it, then you surely must have hacked the system in unsupported ways. ;)

meh. if we're only supporting staged config that originates from code, so be it.

Any of these reference validations will ultimately lead to situations in which the referenced item may not exist before it has been staged/imported. I'm relatively sure that we will face validation dependency scenarios that will require a CONFIG_DEFER equivalent for the validation step, and which will potentially force us to re-run validation before invoking each owner.

in other words, something along the lines of the drupalcon denver import code that was thrown away? if we decide to build that in to this first validation patch, i'll hand the reins to someone else. not interested in getting burnt again.

new patch coming later today.

beejeebus’s picture

wow, i'm so bad at this game. working on an updated patch and i realised i don't know how CMI imports works anymore, and had missed a couple of obvious consequences of the partial import code:

1. it's no longer possible to create new or delete old config files via an import, unless they are managed by a manifest file. lulz. so, no need to worry about building a system to list required files - we've completely taken away the ability to handle create/delete of ordinary config files on import, so we can stop worrying about it

2. it's no longer possible to use the set of changes and config via the source storage controller to validate an import. implementations will have to first look at the set of changes, then look at config via the target storage controller, because there's no source tree they can rely on. more lulz. so we have a couple of options - build a tree ahead of time (based on a merge of the set of changes and the target tree), so that implementations don't have to do it themselves, or just document that you have to look at changes first, then target tree second, to do any useful validation of references and dependencies on import

will wait for feedback before proceeding.

beejeebus’s picture

just discussed this with heyrocker in #drupal-contribute.

for 1), the answer is that's just the way it is, modules should ship all non-configEntity files by default, so there's nothing to do there.

for 2) modules will just have to check two places.

problems solved.

beejeebus’s picture

Status: Needs review » Needs work

i'm no longer working on this, i hope someone else can drive it home.

alexpott’s picture

Assigned: Unassigned » alexpott

I'm going to work on this tonight.

moshe weitzman’s picture

andypost’s picture

So what the state of the issue? seems better to implement special event not the hook...

beejeebus’s picture

Title: Add hook_config_validate() » Implement event listeners to validate imports
Assigned: alexpott » Unassigned
Category: feature » task
Status: Needs work » Active

we have a validation event fired during import, but nothing in core really uses it (we just validate config object names).

so, i guess the remaining bit is to look at subsystems like views and fields etc and implement event listeners that validate imports.

i guess we could turn this in to a meta to track issues for each subsystem?

dawehner’s picture

Opened an issue for views: #2008708: Validate views during import

moshe weitzman’s picture

Title: Implement event listeners to validate imports » [Meta] Implement event listeners to validate imports
Priority: Normal » Major

#1890784: Refactor configuration import and sync functions landed, so we now need to implement validation for Views, Fields, etc. I guess this can be a Meta issue now.

xjm’s picture

So the outstanding tasks for this issue are to actually come up with the specific validation steps that need to be done, e.g., is the module for this configuration installed?

We also should consider validation for config entities in the entity API.

mtift’s picture

Issue tags: +sprint

Tagging

mtift’s picture

Issue summary: View changes

Updated issue summary.

moshe weitzman’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Damien Tournoud’s picture

As I pointed out multiple times, there are (mostly implicit) dependencies between configuration objects. For example, a View might depend on Fields. If we don't have a way to do validation transactionally, ie. validate a set of configuration updates as a whole, we are going to put ourselves in a corner in which many configuration updates cannot possibly to imported.

(In addition, I don't think it's impossible to imagine cross-dependencies between objects. In this case, regardless of the import order you will never be able to import a change set)

alexpott’s picture

Issue tags: +beta blocker
catch’s picture

Priority: Major » Critical
fago’s picture

edited: got confused by the issue title, please ignore.

fago’s picture

We also should consider validation for config entities in the entity API.

Yeah, imo we should solve #1928868: Typed config incorrectly implements Typed Data interfaces, which would allow us to use symfony validator with config entities and config generally also. Also see #2002174-5: Allow vocabularies to be validated via the API, not just during form submissions.

moshe weitzman’s picture

Issue summary: View changes

I think that #2,#3 are still valid validations that don't have nasty dependencies. That is, we can make progress here without having to solve cold fusion.

alexpott’s picture

Priority: Critical » Major
Issue tags: -beta blocker

We have a validation event. And it works and is tested :) see #2056445: Ensure that all config can not be deleted using the config importer. Removing the beta blocker tag since adding listeners to this event is not beta blocking - although I consider it important. Also we have made the decision to only support importing full configuration trees which means that validation should be less of an issue.

xjm’s picture

Issue tags: -sprint
alexpott’s picture

clemens.tolboom’s picture

I came from #2392633: [META] Allow all views plugins to validate and #1996130: REST views: not adding dependencies on Serializer providers looking for clues how to view[type]::validate(). This issue needs some review.

  1. +++ b/core/includes/config.inc
    @@ -195,6 +195,47 @@ function config_sync_changes(array $config_changes, StorageInterface $source_sto
    +  $hook = 'config_import_validate';
    

    Why use a single used variable $hook?

  2. +++ b/core/includes/config.inc
    @@ -195,6 +195,47 @@ function config_sync_changes(array $config_changes, StorageInterface $source_sto
    +      module_invoke($module, $hook, $changes, $source_storage, $target_storage);
    

    module_invoke is marked deprecated.

xjm’s picture

Status: Needs review » Active

A patch from 2012 is really not relevant, so setting back to active. It's a meta in any case; we usually only post and review patches in the child issues.

xjm’s picture

xjm’s picture

xjm’s picture

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Task » Plan
Status: Active » Postponed
Issue tags: -Needs tests

We have improved some of this; the rest shall wait until 8.1.x since it can be done with BC additions.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.