Problem/Motivation
SdcPropToFieldTypePropMatcher is doing a lot.
Proposed resolution
Try to refactor SdcPropToFieldTypePropMatcher to smaller chunks, e.g. event listeners or tagged services.
In order to simplify, perhaps it should use an event or a series of weighted matchers instead of hard coding assumptions about various field types. This would allow other modules like to participate (e.g. DER is a different kind of ER field) but also would make the code a bit easier to understand
👆 Descoped because as of #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings, we can significantly reduce what it is that SdcPropToFieldTypePropMatcher does.
Per https://wimleers.com/xb-week-3, titled shape matching
, this represents an area in the codebase of its own.
In this MR (🙏 please check the commit history, I carefully crafted the commit history to make it easier to review + provided comments on the MR to contextualize specific bits):
- First capture the intent: clearer
CODEOWNERS+ issue queue components: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... - Then move the crufty old classes into a directory structure that matches that first step: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
- OUT OF SCOPE: renaming classes — that'd be too painful to review, that can easily happen later 👍
- Remove all the ancient test expectations from
SdcPropToFieldTypePropTestthat #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings made obsolete/irrelevant (because unused): https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... - (Then update test expectations etc. to get it to green)
- Remove the now obsolete
typesfrom `FieldForComponentSuggester`.: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... - Similarly, update
adapter_matches_field_typeexpectations inSdcPropToFieldTypePropTestto expect only the single `StorablePropShape` expression. - 🔥🪦Dead code removal thanks to #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings in the first place and then the above commits having removed reliance on lower-level infrastructure, that is then now finally dead. 👈 This is where
SdcPropToFieldTypePropMatcherbecame a LOT simpler! - Docs expanded/improved: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
Result: 34 files, +253, −1017.
User interface changes
None.
Issue fork experience_builder-3450496
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 #2
larowlanAh looks like https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... might be just that?
Comment #3
wim leers@larowlan I don't think so — that's adding one missing concept: adapters. It touches the matcher, but won't refactor it.
Quoting my response from the MR where you raised this:
— https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
Thoughts?
Comment #4
wim leersComment #6
tedbowWill give this a try
Comment #8
tedbowComment #9
tedbowPutting this down to work on #3456008: [later phase] Support matching enum SDC prop shapes against DynamicPropSources, not only generating StaticPropSources
Comment #10
wim leersWith #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings in, this needs a significant change.
Comment #11
tedbow#3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings is done
Comment #12
wim leersWith #3461499 done, I'd like to both clean this up and update
CODEOWNERS.Comment #15
wim leersComment #16
wim leersComment #17
wim leersVoila!
Comment #18
wim leers(Assigned to @tedbow because he led the work on #3469436: Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property. And before that, he reviewed #3471171: Allow components to use textarea in favor of input. Retroactively moving both to the new issue queue component.)
Comment #19
wim leersWhile Ted is sleeping, let’s get Felix’ input already 😊
Comment #21
wim leersI'm baffled that @f.mazeikis has zero comments, zero nitpicks 😅
Maybe he'll post those later.
Either way: now getting @tedbow's +1! 🤞
Comment #22
f.mazeikis commentedWell, the MR is mostly clarifying, removing dead code and updating docs. I don't have any nitpicks 😅
My only comment is that perhaps we should file an issue for #note_374141, just so we don't lose track of that it still needs to be addressed.
Comment #23
wim leersGood point! I wonder why that comment of yours on the MR doesn't show up here in the d.o issue?! 😬 Issue created: #3473822: [later phase] Support Drupal's `timestamp` field type + widget?.
Comment #24
f.mazeikis commentedYour self-review questions on MR and contextualisation of commits was really helpful.
There was a lot of context and information to go through, so it took some time.
But at the end I feel like I know much better what is happening in that MR and in general with Props/Shapes/JsonInterpreter related part of backend.
Hence the lack of nitpicks - it is a significant improvement to clarity when it comes to namespacing, docs and inline docs.
Comment #25
tedbowThanks for all the notes @wim leers!
Comment #26
wim leersThanks!
I'll bypass the need for approval for
/*.info.ymlsince literally all it's changing is 2 comments (updated FQCNs).Next up: #3468112: Document the current component discovery + SDC criteria + `Component` config entity, and describe in an ADR.
Comment #28
wim leers