Problem/Motivation
Split off from #3364108: Configuration schema & required keys.
Introduce Mapping::getValidKeys(), ::getRequiredKeys(), etc.
For full history of how we (@bircher and I) arrived at this, see #3364108: Configuration schema & required keys.
Unblocks
- core: #3364108: Configuration schema & required keys
- contrib: https://www.drupal.org/project/config_split → #3390285: Use required keys of mappings instead of special casing
- contrib: https://www.drupal.org/project/config_inspector
Proposed resolution
Mapping::getValidKeys()Mapping::getRequiredKeys()Mapping::getOptionalKeys()Mapping::getDynamicallyValidKeys()
Remaining tasks
Review.
User interface changes
None.
API changes
New methods on \Drupal\Core\Config\Schema\Mapping:
Mapping::getValidKeys()Mapping::getRequiredKeys()Mapping::getOptionalKeys()Mapping::getDynamicallyValidKeys()
New method public TypedConfigManager::findFallback(), which is the public alias of HEAD's protected TypedConfigManager::getFallbackName(). That one remains around, unchanged, to avoid disruption for any contrib/custom subclasses of TypedConfigManager.
(This new public method is necessary for Mapping::getDynamicallyValidKeys().)
Data model changes
None.
Release notes snippet
None.
Issue fork drupal-3401883
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #6
wim leersMR coming shortly…
Comment #7
wim leersMR live.
Note that per #3364108-73: Configuration schema & required keys, this is missing test coverage in
MappingTestfor%key.Comment #9
wim leers[%parent.%parent.something], in addition to the test coverage for[%parent.something]I had already added.I believe this is ready for final review now.
Comment #10
borisson_I think the test coverage for this is now complete, I can't find anything I would change about this, and it makes reviewing/implementing the parent issue a lot simpler.
There is already a big discussion over in the parent issue on how we ended up here as well, so while this issue is missing the complete story, there is more than enough documentation/information to be found there, I'm not sure if we should copy some of the information from there to this issue's summary to make it more clear?
In any case, I think codewise this issue looks good to go in my opinion
Comment #11
alexpottGoing to put this back to needs review as it has received significant changes post rtbc.
Comment #12
wim leersThanks, was about to do that after being deep in the code for hours 🙈
Comment #13
wim leersComment #14
phenaproximaI don't see much to complain about here. My feedback is asking for readability improvements, if possible, in the very complex code. But even with that, it's well-documented, and could be refactored later if needed - especially because there's such extensive test coverage.
I feel okay RTBCing this, since it's a major blocker. I only wrote some of the code comments, but didn't write any of the logic.
Comment #15
wim leersIncorporated all feedback 👍
Comment #16
alexpottCommitted 007c0c7 and pushed to 11.x. Thanks!
Comment #18
wim leersFollow-up: #3406478: Refine ValidKeysConstraintValidator's messages: use #3401883's new Mapping infrastructure.
Comment #20
dwwI doubt folks will see this comment since this issue is already closed, but FYI: #3418691: ckeditor5 module has an invalid config schema which causes POTX to fail