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.
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
- Site config exported to a git master branch.
- Developer A: Deletes the only instance of a field, exports, and commits this git on a branch dev-a
- Developer B: Uses this field in on another content type, exports and commits this git on a branch dev-b
- Team lead: Merges dev-a and dev-b to master
- 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
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. |
Comment | File | Size | Author |
---|---|---|---|
#51 | 2416109-bircher.51.patch | 34.19 KB | alexpott |
#51 | 45-51-interdiff.txt | 14.87 KB | alexpott |
#45 | 2416109-bircher.45.patch | 30.63 KB | alexpott |
#45 | 42-45-interdiff.txt | 11.76 KB | alexpott |
#45 | Screen Shot 2015-05-13 at 17.47.09.png | 175.82 KB | alexpott |
Comments
Comment #1
xjmComment #2
alexpottHere'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.
Comment #4
alexpottI'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.
Comment #5
catchShouldn't the View either get deleted, or implement the self-clean-up method, then?
Comment #6
alexpott@catch what's the UI for that? You click delete on an entity... anyway opened #2416409: Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted to explore this.
Comment #7
xjmI think we should at least report them?
Comment #8
alexpottWell #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
Comment #9
alexpottPatch with tests :)
Also @webflo pointed out to me in IRC that we need validate simple config too - which I've added.
Comment #10
alexpott@bircher identified another way that this is useful.
Comment #12
bircherYes, 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.
Comment #13
bircherin 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.
Comment #14
bircher"needs review" was just for the test-bot.
Comment #15
bircheradding assertion in test.
Comment #16
alexpottWe have tests
Comment #17
alexpottFixing method scope and adding beta evaluation.
Changing to critical for the reasons mentioned in the beta evaluation.
Comment #18
alexpottAs per #17
Comment #19
dawehnerIts 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.
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?
FQCN
Is there a reason we don't use
Long term question | OT ... would it make sense to have it available which file is wrong using same parameter in logError?
Can we make a comment why we need to check the UUID here?
2 whitespaces after be ...
Don't we use $this->pass() in that case? The code reads like it should be ->fail() though ?
FQCN
I'd consider 256 still a quite high priority.
Comment #20
alexpottComment #21
alexpottComment #22
alexpottThanks for the review @dawehner!
listInfo()
- injected the theme handlerre
Yes maybe. Shall I open a followup for this?
Comment #23
dawehnerIt seems to be worth to discuss, here is a follow up: #2486195: Make config import log errors more machine readable
Comment #24
alexpottTo address #19.2 opened #2486199: Move rename validation in ConfigImport to ConfigImportSubscriber
Comment #26
alexpottSo #19.4 so we need all themes not only installed themes.
Comment #27
dawehnerAh another reason why we want #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList :p
Comment #28
jibran@dawehner so can we make #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList an critical task now?
Comment #29
dawehnerA snarky comment also needs a RTBC
Comment #30
alexpottRe #28 - nope.
Comment #31
xjmTaking a look at this.
Comment #32
xjmComment #33
xjmAlrighty! 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.
site_config
directory and rungit init --bare
.site_config
repo into site A's staging directory.site_config
repo into site B's staging directory as well./admin/structure/types/manage/article/fields/node.article.field_tags/delete
.drush cex -y
,git add -u
in the staging directory, commit, and push.cex -y
. Add and commit the changes in the staging directory.git pull
. The changes from site A are automatically merged./admin/config/development/configuration/
. Nothing informs me that I'm about to delete data from my page nodes.You broke all your page nodes in a way that is unrecoverable, congrats! So yeah, this is a critical.
With the patch
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.)
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.)
Comment #34
alexpottPatch 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.
Comment #35
jibran@param type missing :)
Comment #37
alexpottFixed the test and #35.
Comment #38
dawehnerShould we validate that this file actually exists? No idea how hell freezed that this happened but checking for it, seems reasonable.
Do you mind adding a typehint?
Is it worth to extract that duplication into its own protected method, so its not required to do things twice?
Nice fix!
Comment #39
alexpottAlso tried to address the error message issues described by #33.1. See image below:
Comment #40
dawehnerLooks perfect for me now, but I guess we need some additional feedback from xjm here?
Comment #41
catchVery minor nit but:
array_keys(array_diff_key(..))?
Nothing else to complain about.
Comment #42
alexpottNice catch @catch - that is totally an @alexpott-ism :)
Comment #43
dawehnerAwesome!
Comment #44
xjm@alexpott is working on some additional improvements on the messages per our discussion.
Comment #45
alexpottDisplay which modules, themes and modules are missing when configuration dependencies validation fails.
Comment #46
dawehnerYou see the structure ... of why we need a extension list mechanism :P
Comment #47
dawehnerAnyway, one nasty comment, and night and one RTBC
Comment #48
catchGiving xjm a last chance to review this one, agreed those messages are more useful.
Comment #49
alexpott@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.
Comment #50
xjmReviewed 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
Hm. Look at why this method was called before and why it no longer is.
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.Why is it we need to inject the theme handler but not the module handler?
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. :)
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.
Why is this redundant with the other hunk above in
ConfigImporter::createExtensionChangelis()
?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.)
I would say "is still installed" maybe? Not important. Maybe not.
Why is this "existing" dependencies? The source is the staged config, no?
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.
Don't we have an API method for this yet?
So obviously, we don't need to check this for core because core is always installed. Cool.
Minor: By having 'dependencies' and 'uuid' keys?
Nit: human-readable. But also, aren't they just "labels" or something? Or whatever the key is in info files.
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.
Note to self: Read this test.
The wording is a bit weird, but also I can't come up with better.
Can we add assertions that will fail if node no longer depends on text, or bartik no longer depends on classy?
So this is ever so slightly out of scope, but sure. ;)
Thank you for adding these references, +1.
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.
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.Comment #51
alexpottThanks @xjm for the great review.
$this->extensionChangelist = $this->processedExtensions = $this->getEmptyExtensionsProcessedList();
$dependencies_that_will_exist
- obviously open to suggestions thoughComment #52
alexpottre #50.8 maybe a review of all the messages possible would be good I'll post a summary of all them.
Comment #53
dawehnerGIve it another read, even I'm still not confident that I have enough knowledge about it, I trust xjm and alexpott.
Comment #54
xjmThis 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!
I do declare! :) I made some very small wording and punctuation fixes here and elsewhere on commit: