Overview

Proposed in #3461499-17: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings, +1'd by @lauriii in #19 of that issue.

🛑 Blocked on #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings.

For a while now, @lauriii had expressed concerns about the whole "opting components in" UX (even though this was what we decided early on). Only now we're sufficiently confident that we can do without this, based on:

  1. what #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings proved to be possible
  2. #3462705: [META] Missing features in SDC needed for XB becoming gradually more clear, and especially @lauriii specifying that it's okay for XB to only allow SDCs that define >=1 example for every required prop, and accepting that this has consequences for per-site default values

Combining the ability to compute field type + widget (point 1 above) and default value (point 2 above) means that the crucial information that the Component config entity used to gather, no longer needs to be gathered.

Proposed resolution

  1. Automatically generate Component config entities for all SDCs that meet the requirements, upon:
    1. Module installation
    2. Theme installation
    3. Clearing all caches
    4. Bonus/optionally: on every request if Twig development mode is enabled at /admin/config/development/settings
  2. For now those criteria are simple:
    1. every required prop must have >=1 example.
    2. every prop (optional or required) must have a title → surfaced by #3467162: Follow-up for #3461422: display SDC prop's human-readable name (`title`), not its machine name

    (Future criteria: SDC's status must/must not be a certain value, enum values must have labels, etc.)

  3. Use the exact same Component config entity that exists today, so including populating the following:
    defaults:
      props:
        <prop name>:
          field_type: …
          field_widget: …
          default_value: …
          expression: …
    

    (this enables #3464042: Add ComponentAuditabilityTest)

  4. When: unfortunately, \Drupal\Core\Theme\ComponentPluginManager does not specify a cache tag. So, we have no way of knowing when it discovers new SDCs 😬
  5. Work-around: decorate that service, pass everything through, but decorate getDefinitions(). Call the decorated method first, and compare the discovered definitions with those that have Component config entities already. Any that meet the requirements but do not yet have a Component config entity, get a Component config entity.
    (Q: Why ::getDefinitions() and not getAllComponents()? A: because then we avoid instantiating all SDC plugins. But if the latter is simpler for now, then just go for that. Actually, maybe that's acceptable because sdc_library_info_build() calls it too, and a module getting installed would rebuild libraries anyway.)
  6. Delete all experience_builder.component.*.yml files from the codebase — these should be auto-generated now! (This might require some shenanigans/fiddling in tests, especially in kernel tests.)

User interface changes

None — other than /admin/structure/component maybe having more things listed :)

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

wim leers’s picture

Title: Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria » [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria
Issue summary: View changes
wim leers’s picture

Issue summary: View changes
wim leers’s picture

Issue summary: View changes
wim leers’s picture

Issue summary: View changes
wim leers’s picture

Assigned: Unassigned » lauriii
Issue summary: View changes
Related issues: +#3464042: Add ComponentAuditabilityTest

Asked @lauriii to confirm he agrees with the proposed resolution, especially with an eye towards #3464042: Add ComponentAuditabilityTest.

lauriii’s picture

Assigned: lauriii » Unassigned

I responded on #3464042: Add ComponentAuditabilityTest. I'm concerned that storing this information in the Component config impacts DX negatively. We need to consider how are components authored, and how are they edited during their lifecycle, and how that overall experience looks like. Today, if components or templates are being changed, the changes are reflected immediately to the site. We should strive that same experience. We need to remove as many barriers from the XB from authors; the goal is for it to feel just as if you're editing a regular template or component.

wim leers’s picture

Today, if components or templates are being changed, the changes are reflected immediately to the site.

This is not true:

  1. To have template changes reflect immediately (including SDC template changes), you'd need to enable Twig development mode at /admin/config/development/settings
  2. To have component changes (aka component metadata changes) reflect … there's no way to do that, not even https://www.drupal.org/project/cl_devel does that. The only way to achieve that is to manually clear the discovery cache bin's component_plugins cache item! And since there's no cache tags associated with that, the only way to achieve that is to go to /admin/config/development/performance and click Clear all caches (more efficiently through the CLI: drush cc discovery).
    Actually … the way it's cached is so aggressive that a major core bug for it was just fixed yesterday: #3460598: Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear. 😅

IOW: to actually achieve what you describe, we'd need an SDC development mode, which re-discovers all SDC metadata on every request. We could and should do that! Opened issue for that: #3464388: SDC *.component.yml metadata is cached aggressively, gets in the way of component development.

And … when that mode is on … we'd also just automatically update the Component config entity! 🤓

Result:

  1. the exact DX you describe in #8
  2. the auditability described in #3464042: Add ComponentAuditabilityTest
wim leers’s picture

Issue tags: +blocker

This blocks #3464042, and at #3464042-4: Add ComponentAuditabilityTest you can find a detailed write-up that justifies both this issue as well as that one, based on XB's product requirements.

wim leers’s picture

Title: [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria » Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria
Assigned: Unassigned » f.mazeikis

#3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings is in.

@f.mazeikis, see #3461499-24: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings for some additional context, but here's the most relevant bit:

In the MR, there's 4 @todos pointing to #3463999: Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria, which is where the automatic fallback logic that this added can be removed. "Automatic fallback logic" is a nice alternative phrasing for "magic 🧙", and we should avoid it. Removing that magic is an order of magnitude simpler once these component config entities are fully auto-generated. So, deferring that part to that issue 👍

wim leers’s picture

Issue summary: View changes
wim leers’s picture

AFAICT this issue would likely have independently surfaced #3465981: Follow-up for #3461499: transform at-rest field storage settings using FieldItemInterface::settingsFromConfigData(), especially if #3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree" lands before this.

#3465981 is only a soft blocker to this though, because there's plenty of other pieces to be done in this issue.

f.mazeikis made their first commit to this issue’s fork.

wim leers’s picture

wim leers’s picture

Issue summary: View changes
wim leers’s picture

Title: Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria » [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria

Discussed in detail with @f.mazeikis. He discovered oversights in (Candidate)StorablePropShape + the config schema while working on this MR. The solution is evident: a fieldInstanceSettings sibling to fieldStorageSettings. He'll extract that as a blocking issue, with a fairly trivial MR that can land very quickly, to unblock this issue to do things "properly". 👍

wim leers’s picture

Title: [PP-1] Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria » Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria
Status: Postponed » Active

That's in now! 🚀

larowlan’s picture

Rather than this being automated, what about an import wizard from the list builder as an action link? When we support blocks that could also be included

wim leers’s picture

#20:

@lauriii has stated it's a hard requirement that it is possible to do it fully automatically.

As a next step, we would only do it fully automatically when in "development mode" (because on production you wouldn't want new SDCs appearing in XB. See #3464042: Add ComponentAuditabilityTest, and the discussions with @catch on that subject over at #3444424: [META] Configuration management: define needed config entity types.

wim leers’s picture

Reviewed 🏓

The failing Cypress E2E tests are showing this in the output:

{
                      "status": "PARSING_ERROR",
                      "originalStatus": 500,
                      "data": "The website encountered an unexpected error. Try again later.<br><br><em class=\"placeholder\">Twig\\Error\\RuntimeError</em>: An exception has been thrown during the rendering of a template (&quot;[activation] String value found, but an array or an object is required&quot;). in <em class=\"placeholder\">Twig\\Template-&gt;yield()</em> (line <em class=\"placeholder\">1</em> of <em class=\"placeholder\">modules/custom/experience_builder/components/containers/shoe_tab_group/shoe_tab_group.twig</em>). <pre class=\"backtrace\">Drupal\\Core\\Template\\ComponentsTwigExtension-&gt;doValidateProps(Array, &#039;experience_builder:shoe_tab_group&#039;) (Line: 109)\nDrupal\\Core\\Template\\ComponentsTwigExtension-&gt;validateProps(Arr ...

… and sure enough, components/containers/shoe_tab_group/shoe_tab_group.component.yml does look rather bizarre:

    activation:
      type: array
      title: Activation
      description: When set to auto, navigating tabs with the arrow keys will instantly show the corresponding tab panel. When set to manual, the tab will receive focus but will not show until the user presses space bar or enter.
      default: auto
      examples: ['auto', 'manual']

I went back to #3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree" and found that @ctrlADel's original implementation of that (commit: ef34f8106dfe2103df38055154efc9bef9ba9b1d) was never changed by @tedbow nor me. So I think this was just an honest mistake.

Fixed in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

wim leers’s picture

A bunch of the components that @ctrlADel added in #3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree" do have examples but then specify no examples. That's odd/weird, but XB should graciously handle that: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

wim leers’s picture

Status: Active » Needs work
Issue tags: +Needs tests

I pushed a handful of tiny commits to help get this MR to green — because the failures were happening in areas far removed from the changes that are actually in the scope of this MR (and far outside areas @f.mazeikis has previously touched).

I also solved one very tricky thing WRT default config: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1....

This MR still needs:

  1. test coverage (see https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...)
  2. to address remaining review feedback
  3. conflict resolution

… but those should be smooth sailing now 😊🤞 Looking forward to seeing this one land! 😄

wim leers’s picture

Title: Auto-generate Component config entities for all discovered SDCs that meet XB's minimum criteria » Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria
Issue tags: -Needs tests

Tests look great!

I bet fixing https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... will also fix the failing Cypress E2E test.

This feels very close now 😊🥳

wim leers’s picture

Assigned: f.mazeikis » Unassigned
Status: Needs work » Reviewed & tested by the community

This issue represents a huge leap forward! 🚀

Most importantly in the short term: this will make it an order of magnitude easier to start testing https://www.drupal.org/project/demo_design_system 👍

Also:

  1. This unblocks #3464025: Remove Component config entity's add/edit form, which likely will be quite satisfying for @f.mazeikis to do :)
  2. It also unblocks the follow-up #3469461: New component requirement: each SDC prop must have StorablePropShape, which will also help testing https://www.drupal.org/project/demo_design_system
  3. This made me realize that we should do (the newly opened) #3469609: Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs ASAP for a consistent data model.

wim leers’s picture

Status: Reviewed & tested by the community » Fixed
wim leers’s picture

Component: Page builder » Config management
wim leers’s picture

Status: Fixed » Closed (fixed)

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