[Meta] Implement event listeners to validate imports

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.

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.

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.

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.

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.

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.

+++ 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.

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 :/

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

+++ 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)

I'm going to work on this tonight.

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

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

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.

Tagging

Updated issue summary.

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)

Should this be postponed on #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall?

Bumping to critical either way.

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

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.

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.

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.

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.

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