Reviewed & tested by the community
Project:
Configuration Split
Version:
2.0.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
27 Sep 2023 at 22:14 UTC
Updated:
3 Feb 2026 at 12:33 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
bircherAttached is patch for making it work.
The tests also need to be updated to make the config validation happy.
Comment #3
wim leersThis looks nice & simple! :D
Do you intend to adopt
Mapping::getConditionallyValidKeys()here? Or is that planned for this issue?Comment #4
bircherThe annotation of the method I want to use is:
Gets all required keys in this mapping.That is exactly what I want.I don't care which keys are maybe required in a Mapping if it had other data. All I care is the current Mapping with the current data has some required keys and I can use those.
Maybe I should add a test for the case when a key is required in some cases. But the API of
getConditionallyValidKeysdoesn't seem very useful in this case. I understand it is very useful in error messages.Comment #5
wim leersYou're right that the thing that truly matters to Config Split, is only the required keys. 👍
But … that doesn't mean that some config does not have some invalid keys when merging? IOW: I think that #2 is 99% of the value/work, but I do think that the remaining 1% is also important: validating that the result of merging contains only valid keys.
So I was wrong in #3: you indeed do not need to adopt
Mapping::getConditionallyValidKeys(). But AFAICT you do need/wantMapping::getValidKeys(). Although perhaps there is something about the way that Config Split is used that you'd need to do some extremely impractical splitting before you could hit that edge case?Comment #6
bircherYes you are right of course! getValidKeys would be useful here. But you are also right that this is quite an edge case for config split.
So I prefer not to do anything about the invalid keys now, let core complain about it and the user opening an issue with steps to reproduce it. Then we can investigate why that could happen.
Comment #7
wim leers👍
Comment #8
bircherI am postponing this for the core issue to be committed.
Comment #9
wim leers#3401883: Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. has been committed and will ship in 10.3 🚀
Comment #10
adam-vessey commented#2 seems to get into issues should the config_split patch attempt to fully remove a mapping from a sequence, say for example: A field defined in an entity_form_display that was hidden. On merging, it would re-add an unconfigured/empty mapping with only the required array properties, which in turn has the form for configuring the display throwing warnings regarding missing `weight` and `type` values.
A bit of investigation, it looks like passing the "parent type" down to know if suppressing the given value as a whole is appropriate might do the trick. Will look into getting an MR together.
Comment #13
adam-vessey commentedAdjusted approach, got MR together with tests around the target functionality.
Ended up removing the behavior added in #3295853: [PP-2] Split config removes settings and third_party_settings, leads to fatal TypeError and moving instead to implement on the "merge" side of things. Tests locally against D10 seem to pass fine; however, those in the pipeline were running against D11, which seems to have diverged in the signature of some of the services in use (Since D10.1: #3284397: ConfigImporter requires the theme extension list to be injected)... made adjustments adjustments to pass the additional service along in the constructor, and things seem to be fine.
Comment #14
adam-vessey commentedComment #15
jordandukart commentedIntroduction of requiredKeys in 10.3 + this patch has solved issues with keys disappearing on export in certain scenarios denoted above. Changes to the merge behavior should be fine given the internal nature of the class and removal of functionality as noted in #13.
Comment #16
basvredelingThe patch from MR53 no longer seems to apply cleanly to config_split 2.0.2. Needs a re-roll.
Comment #18
frocha commentedComment #19
frocha commentedI just updated MR. Now the diff file works in version 2.0.2.
Comment #20
basvredeling#17 looks good.
Comment #23
tgoeg commentedhttps://git.drupalcode.org/project/config_split/-/commit/c4d6f474c72f60d... already adds the last patch in the MR, that's why it could not be used as a patch in composer.json
I rebased (or better, merged in master), the patch can now be used in composer.json like this if anyone wants to try:
It fixes #3547143: Remove empty arrays on removing config patch for me as well.