Use required keys of mappings instead of special casing

Created on 27 September 2023, over 1 year ago
Updated 27 November 2023, about 1 year ago

Problem/Motivation

On https://git.drupalcode.org/project/config_split/-/blob/2.0.x/src/Config/...
There is a line saying: // @todo find a better way to know which elements are required.

📌 Configuration schema & required keys Fixed is a dedicated issue in core to fix that.

Proposed resolution

use the solution from 📌 Configuration schema & required keys Fixed

Remaining tasks

User interface changes

none

API changes

none

Data model changes

applied patches will respect the config schema of when to retain required keys.

📌 Task
Status

Needs work

Version

2.0

Component

Code

Created by

🇨🇭Switzerland bircher 🇨🇿

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @bircher
  • Status changed to Needs review over 1 year ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 8
    last update over 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/want Mapping::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.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    👍

  • Status changed to Postponed over 1 year ago
  • 🇨🇭Switzerland bircher 🇨🇿

    I am postponing this for the core issue to be committed.

  • Status changed to Needs work about 1 year ago
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    📌 Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. Active has been committed and will ship in 11.3 🚀

  • 🇨🇦Canada adam-vessey PE, Canada

    #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 get an MR together.

  • Merge request !53Resolve #3390285 "Use required keys" → (Open) created by adam-vessey
  • Pipeline finished with Failed
    2 days ago
    Total: 207s
    #408749
  • 🇨🇦Canada adam-vessey PE, Canada

    adam-vessey changed the visibility of the branch 3390285-use-required-keys to hidden.

  • Pipeline finished with Failed
    2 days ago
    Total: 211s
    #408752
  • Pipeline finished with Canceled
    2 days ago
    Total: 73s
    #408756
  • Pipeline finished with Success
    2 days ago
    #408788
  • 🇨🇦Canada adam-vessey PE, Canada

    Adjusted approach, got MR together with tests around the target functionality.

    Ended up removing the behavior added in 🐛 [PP-2] Split config removes settings and third_party_settings, leads to fatal TypeError Closed: duplicate 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])... made adjustments adjustments to pass the additional service along in the constructor, and things seem to be fine.

  • 🇨🇦Canada adam-vessey PE, Canada
Production build 0.71.5 2024