Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #30
Invalid Config can get imported and hose your site
Proposed resolution
Implement validate hooks
Remaining tasks
- #1862160: config() "on not found" behaviour flag is necessary
- 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.
- Incoming views should be validated
API changes
None
Original report by @moshe weitzman
Comment | File | Size | Author |
---|
Comments
Comment #1
sunThanks 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.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedYes, I agree with all of those assumptions.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedyay 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.
Comment #4
sunOk, 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.
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #6
sunThat's an interesting idea — I'm not sure whether this counts as easy or complex:
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?
Yeah, it's probably a good idea to design this from a hook implementator perspective.
Comment #7
gddI 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.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedAnyone up for writing an initial patch here?
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedjust 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.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedand here we go.
- missing tests
- no idea how to validate views display plugins...
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commented- fixed missing FileStorage in system.module
Comment #13
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #14
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #15
dawehnerI 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.
Comment #16
damiankloip CreditAttribution: damiankloip commentedThe 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 :/
Comment #17
dawehnerThen this is a bug, we really should check that.
Comment #18
sunWouldn'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.
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?
Unused.
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?
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?
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. ;)
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)
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedthat 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.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedwow, 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.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedjust 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.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm no longer working on this, i hope someone else can drive it home.
Comment #23
alexpottI'm going to work on this tonight.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedThe plan is to add validation after #1890784: Refactor configuration import and sync functions
Comment #25
andypostSo what the state of the issue? seems better to implement special event not the hook...
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commentedwe 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?
Comment #27
dawehnerOpened an issue for views: #2008708: Validate views during import
Comment #28
moshe weitzman CreditAttribution: moshe weitzman commented#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.
Comment #29
xjmSo 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.
Comment #30
mtiftTagging
Comment #30.0
mtiftUpdated issue summary.
Comment #30.1
moshe weitzman CreditAttribution: moshe weitzman commentedUpdated issue summary.
Comment #31
xjmComment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedAs 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)
Comment #33
alexpottComment #34
catchShould 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.
Comment #35
fagoedited: got confused by the issue title, please ignore.
Comment #36
fagoYeah, 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.
Comment #37
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #38
alexpottWe 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.
Comment #39
xjmComment #40
alexpottComment #41
clemens.tolboomI 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.
Why use a single used variable $hook?
module_invoke is marked deprecated.
Comment #42
xjmA 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.
Comment #43
xjmComment #44
xjmComment #45
xjmComment #46
xjmWe have improved some of this; the rest shall wait until 8.1.x since it can be done with BC additions.