Posted by moshe weitzman on November 16, 2012 at 7:18pm
14 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | configuration system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | alexpott |
| Status: | needs work |
| Issue tags: | Configuration system, Needs tests |
Issue Summary
Add new validation hook which runs before importing of config. Any module can reject the import with this hook.
One use case follows on from #1735118: Change notice: 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.
Comments
#1
Thanks for writing down a use-case. That makes sense.
Is it safe to make the following assumptions?
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.
#2
Yes, I agree with all of those assumptions.
#3
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.
#4
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.
#5
@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.
#6
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.
#7
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.
#8
Anyone up for writing an initial patch here?
#9
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.
#10
and here we go.
- missing tests
- no idea how to validate views display plugins...
#11
The last submitted patch, 1842956-config-validate-10.patch, failed testing.
#12
- fixed missing FileStorage in system.module
#13
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.
#14
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.
#15
+++ 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.
#16
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 :/
#17
Then this is a bug, we really should check that.
#18
+++ 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)
#19
that seems fair, i think i was trying to pass as a little around as possible.
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.
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?
meh. if we're only supporting staged config that originates from code, so be it.
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.
#20
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.
#21
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.
#22
i'm no longer working on this, i hope someone else can drive it home.
#23
I'm going to work on this tonight.
#24
The plan is to add validation after #1890784: Refactor configuration import and sync functions