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):

  1. First capture the intent: clearer CODEOWNERS + issue queue components: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
  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...
  3. OUT OF SCOPE: renaming classes — that'd be too painful to review, that can easily happen later 👍
  4. Remove all the ancient test expectations from SdcPropToFieldTypePropTest that #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...
  5. (Then update test expectations etc. to get it to green)
  6. Remove the now obsolete types from `FieldForComponentSuggester`.: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...
  7. Similarly, update adapter_matches_field_type expectations in SdcPropToFieldTypePropTest to expect only the single `StorablePropShape` expression.
  8. 🔥🪦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 SdcPropToFieldTypePropMatcher became a LOT simpler!
  9. Docs expanded/improved: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

Result: 34 files, +253, −1017.

User interface changes

None.

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

larowlan created an issue. See original summary.

larowlan’s picture

wim leers’s picture

@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:

Eventually, maybe. Right now, we should first explore what the shape is of the needed solution.

In this particular class though I'm not convinced yet we'd need an event/what the benefit would be. Because this is 100% reusing what's in `SdcPropToFieldTypePropMatcher`, it just adds heuristics to generate a sensible ordering. Events implies many pieces of logic collaborating, but that'll result in heuristics fighting/conflicting. In this particular situation, I think replacing the service would be more appropriate.

— https://git.drupalcode.org/project/experience_builder/-/merge_requests/2...

Thoughts?

wim leers’s picture

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

tedbow’s picture

Assigned: Unassigned » tedbow

Will give this a try

tedbow’s picture

Status: Active » Needs work
tedbow’s picture

wim leers’s picture

Assigned: Unassigned » wim leers
Status: Needs work » Postponed
Issue tags: +Needs issue summary update
tedbow’s picture

wim leers’s picture

Title: Refactor and break up SdcPropToFieldTypePropMatcher » Clarify the "shape matching" bits
Issue summary: View changes
Related issues: +#3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings

With #3461499 done, I'd like to both clean this up and update CODEOWNERS.

wim leers’s picture

Title: Clarify the "shape matching" bits » Clarify the "shape matching" bits: namespaces, `CODEOWNERS` and as issue queue component
wim leers’s picture

Assigned: wim leers » tedbow
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
wim leers’s picture

Component: Data model » Shape matching
Priority: Normal » Critical

Voila!

wim leers’s picture

Assigned: tedbow » f.mazeikis

While Ted is sleeping, let’s get Felix’ input already 😊

wim leers’s picture

Assigned: f.mazeikis » tedbow

I'm baffled that @f.mazeikis has zero comments, zero nitpicks 😅

Maybe he'll post those later.

Either way: now getting @tedbow's +1! 🤞

f.mazeikis’s picture

Well, 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.

wim leers’s picture

Good 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?.

f.mazeikis’s picture

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

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for all the notes @wim leers!

wim leers’s picture

Assigned: tedbow » Unassigned

Thanks!

I'll bypass the need for approval for /*.info.yml since 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.

  • wim leers committed 0da3bb4a on 0.x
    Issue #3450496 by wim leers, tedbow, f.mazeikis: Clarify the "shape...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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