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.
We're adding validation to the config import but the single import form is not validated at all. For example we should validate dependencies before importing - #2416109: Validate configuration dependencies before importing configuration
Comment | File | Size | Author |
---|---|---|---|
#36 | 2486467-36.patch | 20.95 KB | alexpott |
#36 | 31-36-interdiff.txt | 11.11 KB | alexpott |
#31 | 2486467-30.patch | 22.98 KB | alexpott |
#31 | 23-30-interdiff.txt | 3.79 KB | alexpott |
#23 | 2486467-22.patch | 23 KB | alexpott |
Comments
Comment #1
kim.pepperLooking
Comment #2
kim.pepperNot looking.
Comment #3
alexpottLet's just use the ConfigImporter proper - then we have no need to do anything special - it just runs the validation AND we get to enable modules by editing core.extension - how cool is that?!?
Comment #4
kim.pepperAt a quick glance, I noticed we have to inject all the dependencies of ConfigImporter into ConfigSimpleImportForm.
Possibly a good candidate for a ConfigImporterFactory so we can hide this?
Comment #5
alexpottI think that if we address #4 it should be done in a followup. I've tried quite hard to keep the ConfigImporter object out of the container because it is a very ugly god object.
Comment #6
xjmDiscussed with alexpott. This patch will make this form much safer to use, and is not disruptive to existing sites, so tagging as an rc target.
Comment #7
cilefen CreditAttribution: cilefen commentedComment #8
xjmComment #9
alexpottRerolled.
Comment #10
alexpottAdded tests. Test only patch is the interdiff.
Comment #13
alexpottFixing the patch. alexpott--
Comment #15
alexpottLet's try getting the test only patch right. Re-uploading the full patch too so that the latest comment has the correct patch.
Comment #17
alexpottOkay 4th time lucky... alexpott-- alexpott-- my .htaccess snuck into the test only patch.
Comment #19
tim.plunkettextra newline
O_O
I know I've thought this before, may have asked it, but why isn't this a service or something? Why must my class have all 7 of these injected?
newline please
I'm not up to date on such things, but dow we really throw drupal_render calls in like this now?
My eyes glossed over at the StorageOverrideWrapper stuff, I'll have to come back.
Comment #20
alexpottThanks @tim.plunkett
Comment #21
mtiftThis looks good to me. Mostly just nitpicks.
Missing a period.
Missing a period.
Missing short description.
Missing docblock.
I'm not sure if this would be the place, or if this would be too much of a rabbit hole, but should we try to throw an error somewhere if the YAML format is incorrect? As it is now, a missing space before the value ("key:value"), or a key without a value ("key"), results in an uncaught PHP exception.
Comment #22
mtiftComment #23
alexpottComment #24
alexpottCreated #2617152: Single import form needs to handle invalid YAML correctly and not with an exception. for #21.5
Comment #25
mtiftLooks good to me
Comment #29
tim.plunkett$this->t()
$batch['operations'][] = [[static::class, 'processBatch'], [$config_importer, $sync_step];
:\ oh well
Comment #31
alexpottThanks @tim.plunkett.
I've replace the rest of the old style array syntax added by the patch...
Comment #33
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWe're in the final home stretch of preparing 8.0.0, and we won't be committing the majority of "rc target" issues any more, but still leaving the tag on them to triage them for patch-release or minor-release post 8.0.0 release. Meanwhile, switching to "8.0.0 target" for issues we'd still love to get in before 8.0.0.
Comment #34
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis patch looks great. Here are some docs nits, but doesn't need to block commit. Leaving at RTBC, and I'll commit later today with or without these addressed, if another committer doesn't beat me to it.
s/Event/The event/
Remove.
It's a bit unclear in this comment that "overrides" in this class means the configuration is fully overridden, rather than the partial overrides we support in ConfigFactoryOverrideInterface.
Copy/paste error from some other code that does caching?
And here.
Comment #35
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRather than duplicating ConfigSync::finishBatch(), should we call it instead?
Comment #36
alexpott@effulgentsia thanks for the reviews.
Re #34
Re #35
Good idea and we can do the same for processBatch too!
Comment #37
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTicking credit boxes for @mtift and @tim.plunkett for reviews.
Comment #38
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.0.x.