- Issue created by @bircher
- Status changed to Needs review
about 1 year ago 10:16pm 27 September 2023 - last update
about 1 year ago 59 pass - 🇨🇭Switzerland bircher 🇨🇿
Attached is patch for making it work.
The tests also need to be updated to make the config validation happy. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This looks nice & simple! :D
Do you intend to adopt
Mapping::getConditionallyValidKeys()
in another issue? Or is that planned for this issue? - 🇨🇭Switzerland bircher 🇨🇿
The 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
getConditionallyValidKeys
doesn't seem very useful in this case. I understand it is very useful in error messages. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
You'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? - 🇨🇭Switzerland bircher 🇨🇿
Yes 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.
- Status changed to Postponed
about 1 year ago 12:07pm 19 October 2023 - 🇨🇭Switzerland bircher 🇨🇿
I am postponing this for the core issue to be committed.
- Status changed to Needs work
about 1 year ago 4:28pm 27 November 2023 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. Active has been committed and will ship in 11.3 🚀