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

  1. core: #3364108: Configuration schema & required keys
  2. contrib: https://www.drupal.org/project/config_split#3390285: Use required keys of mappings instead of special casing
  3. contrib: https://www.drupal.org/project/config_inspector

Proposed resolution

  1. Mapping::getValidKeys()
  2. Mapping::getRequiredKeys()
  3. Mapping::getOptionalKeys()
  4. Mapping::getDynamicallyValidKeys()

Remaining tasks

Review.

User interface changes

None.

API changes

New methods on \Drupal\Core\Config\Schema\Mapping:

  1. Mapping::getValidKeys()
  2. Mapping::getRequiredKeys()
  3. Mapping::getOptionalKeys()
  4. 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

Command icon 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

Wim Leers created an issue. See original summary.

Wim Leers credited bircher.

wim leers’s picture

Assigned: Unassigned » wim leers
Status: Active » Needs work

MR coming shortly…

wim leers’s picture

Assigned: wim leers » Unassigned

MR live.

Note that per #3364108-73: Configuration schema & required keys, this is missing test coverage in MappingTest for %key.

phenaproxima made their first commit to this issue’s fork.

wim leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
  1. @phenaproxima's rephrasings look good 👍
  2. Draft CR created.
  3. Pushed explicit test coverage for [%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.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Going to put this back to needs review as it has received significant changes post rtbc.

wim leers’s picture

Thanks, was about to do that after being deep in the code for hours 🙈

wim leers’s picture

Issue summary: View changes
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

wim leers’s picture

Incorporated all feedback 👍

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 007c0c7 and pushed to 11.x. Thanks!

  • alexpott committed 007c0c77 on 11.x
    Issue #3401883 by Wim Leers, phenaproxima, alexpott, borisson_, bircher...
wim leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

dww’s picture

I 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