- Issue created by @wim leers
- Assigned to wim leers
- Status changed to Postponed
7 months ago 3:09pm 15 July 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
I'll get a config schema going, to clarify the scope and envisioned architecture.
- Issue was unassigned.
- Status changed to Active
7 months ago 4:26pm 15 July 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Similar to what I did at #3452397-8: Allow specifying default props values when opting an SDC in for XB → : attached is the goal :) If more clarification is needed, I could create a first iteration of the config schema too.
- 🇫🇮Finland lauriii Finland
I'm wondering if this is something we would even have to provide as a configuration. Does this change site by site? There could be specific things we'd want users to be able to configure, like which image provider to use. There's probably some other scenarios to account for but does it warrant a whole UI for configuring all of this? Even if we didn't build a whole UI and config system for this, we could allow modules to make more advanced changes programmatically.
I'm also curious if this should act as a default value, or if the change should propagate across the system. Imagine that you had to move from file field to media entity. Why would an user ever want to update every individual component to use media instead of the file field?
How does the default values in
PropShapeInput
work in relation to default values shipped in components? We want the components to behave consistently when they are moved from one system to another, including their default values. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Thought about this some more, and I think I see now where @lauriii was comping from.
- Because this new
PropShapeInput
config entity type didn't exist yet, ✨ Allow specifying default props values when opting an SDC in for XB Fixed had no choice but to create a UI, because people working on the UI would not be able to test additional components otherwise. - (Fortunately, it was able to leverage some of the work that I did for
TwoTerribleTextAreasWidget
.) - But if this
PropShapeInput
config entity type had existed already, then ✨ Allow specifying default props values when opting an SDC in for XB Fixed would likely not have built that UI. - … because 🌱 Milestone 0.1.0: Experience Builder Demo Active won't need that, and even 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active won't need it.
So: descoping that part. Even if it is "as simple as copy/pasting parts from
ComponentEditForm
" — it's extra time and probably there will be gotchas. Let's avoid them. - Because this new
- 🇫🇮Finland lauriii Finland
Yes, it changes site-by-site. One site would want to use the Image field type, another would want to use the Media field type restricted to the "Photo", "Screenshot" and "Flickr" media types.
That's what I tried to say in #5 – there are valid use case like selecting where images come from. However, this is different from selecting whether component uses radio vs select, or textfield vs textarea. In these examples, it is not something that would change site by site, so why provide a UI at all for configuring this?
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
why allow configuring this outside the component at all?
Because SDCs are designed to have no dependencies (other than CSS/JS, and perhaps reuse Twig templates if the SDCs are part of a theme).
To be more precise: SDCs have no way to declare that a particular field type or field widget must be installed.
That's exactly why the
\Drupal\experience_builder\Entity\Component
was introduced in the first place: to capture that information in a structured way that can be A) shared from one site to the other, B) deployed.This issue aims to simplify that, by reducing the amount of choices that must be stored by each individual
Component
config entity, and instead relaying that to this newPropShapeInput
config entity. A dozenPropShapeInput
config entities may be sufficient for all SDCs used in XB, the 95p case is likely a few dozen — for hundreds of SDCs.If your implied point is "but radio/select/textfield/textarea are provided by Drupal "core" core (i.e. not even a core module), so why do we need to care about those dependencies at all?!", then I'd disagree — because A) it's totally reasonable to want to use contrib-provided field types/widgets, B) there are plenty of field types outside "core" core, but by core modules: from images to date range, and much more.
This is closely related to the discussion, you, @catch and I have going on in 🌱 [META] Configuration management: define needed config entity types Active .
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
As described at #3454173-19: [PP-1] Media Library integration → : I think doing this might unblock that directly, which would remove 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed from the critical path.
Hence increasing priority.
- 🇫🇮Finland lauriii Finland
To be more precise: SDCs have no way to declare that a particular field type or field widget must be installed.
Can we allow them to declare that? It seems that we are regressing the DX in a non-trivial way due to constraints that we have within the system.
A) it's totally reasonable to want to use contrib-provided field types/widgets, B) there are plenty of field types outside "core" core, but by core modules: from images to date range, and much more.
I think the image use case is the single common scenario we've identified. Other than that, this feels quite edge casey for components.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Can we allow them to declare that?
Not without fundamentally changing an architectural choice AFAICT.
It seems that we are regressing the DX in a non-trivial way due to constraints that we have within the system.
I do not see how new
PropShapeInput
config entities harm the DX of SDC developers? 🤔 In fact, it's the literal opposite: it means that SDC authors continue to not have to think about neither how their SDC's props would be stored, nor what the input UX would be?[…] this feels quite edge casey for components.
😳 Paraphrased quote from the very first meeting I was in about XB: "bring in the entire existing Drupal ecosystem, including for example when a site owner has done significant investment in making the UX of (custom) field widgets great — throwing that away is not acceptable".
Is that no longer true then?
- Assigned to wim leers
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@lauriii is not yet convinced, but he's not available to discuss it further, so to A) get this moving, B) ground the conversation with @lauriii more, getting started on this nonetheless.
- Status changed to Needs work
6 months ago 2:58pm 24 July 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
I started with a test that gathers all unique SDC prop schemas. Then I started writing logic to automatically suggest the sane core-provided field type choice.
In doing so, I started treading on the territory of #3456008 — and I believe that we now should not do that issue anymore, at least not in its original scope: 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed .
I am now thinking that not using a config entity type might actually be the only viable choice, because the config entity being proposed here only is viable if there is a good way to transform a given
SDC prop schema
into aPropShapeInput config entity ID
— because that's how we could achieve sane dependencies.The only reason that worked in the original proposal, is because HEAD does not yet support the
{type: string, enum: …}
use case, which is AFAICT widespread among SDCs. Once that is considered, the only way to generate a config entity ID is by hashing the enum.The issue title + summary need a drastic update. See the new test class, which has 3 test methods that build on top of each other. I'll continue this MR tomorrow and will update this issue as well as 🌱 [META] Early phase back-end work coordination Active .
- Assigned to lauriii
- Status changed to Needs review
6 months ago 11:22am 25 July 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
The
\Drupal\experience_builder\Form\ComponentEditForm
UX became simpler:Discussion with @lauriii
Yesterday morning, prior to writing #15, @lauriii and I met and discussed this problem space in detail.
The key outcomes of that discussion:
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
examples
array must not be empty, and XB will use the first one. ⇒ Removes the need for 25% of what theComponent
config entity provides. - @lauriii was heavily leaning towards "no UI" and "logic with an alter hook".
- Tricky: complex shapes, like
json-schema-definitions://experience_builder.module/image
— those would need a plugin system to provide the appropriate field type + field storage.
This issue: current MR state and current consequences
This issue proves it's possible to generate the default
field_type
,expression
andfield_widget
value for each prop. ⇒ Removes the need for 75% of what theComponent
config entity provides.This issue did implement it all as logic (i.e. with nothing stored as a config entity — see my comment #15 for why that turned out to be trickier than I thought when I created this issue), with a TODO for an alter hook.
As demonstrated in #16, this means the scope for 📌 [PP-1] Add support for matching SDC prop shape: {type: string, enum: …} Postponed changes to only having to worry about matching existing field instances that can be adapted to match the SDC prop's enum values.
Questions for @lauriii prior to landing this MR
- Is it acceptable for the logic that determines which field type + widget to use to change and hence newly created instances of this component to potentially use a different field type than the pre-existing component instances?
- SDC supports default
*.component.yml
-defined values. But that won't work for e.g. a default image. Is it okay for XB to layer on support for that by auto-discovering apropname.(gif|png|jpg|…)
file in the SDC's directory? 🤔 (Will add this to 📌 [SPIKE] Comprehensive plan for integrating with SDC Active , to potentially add that to upstream SDC.) - Is it acceptable that the default value sourced from a component's
examples
cannot be overridden on a per-site basis?
I think the answer to the first question is "yes". (Note that every stored
StaticPropSource
is self-contained: the user will continue to be able to edit it, even if it's a completely different field type.)If the answer to the second and third questions is "yes", then we can in principle remove the
Component
config entity. But that would introduce a huge risk: it'd make exporting/sharing configuration (in the future: 🌱 [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model Active , but now:config/optional/field.field.node.article.field_xb_demo.yml
, which has config dependencies on theComponent
config entities it uses) brittle.Therefore I think it'd be preferable to keep
Component
config entities under the hood — without showing any of that in the UI. I think that would also keep the door open for supporting per-site default values in the future, plus it will likely help with 🌱 [META] Support component types other than SDC Needs work .If you agree, then I'll create three follow-up issues:
- one that generates
Component
config entities for all discovered SDCs 1) upon installing XB, 2) upon installing a new module or theme - one to remove
\Drupal\experience_builder\Form\ComponentEditForm
after that auto-generation is happening - one for computing the appropriate field type + storage for
$ref
-defined prop shapes
- XB will require that an SDC provides an example value for it to be surfaced in XB. IOW: the
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Oh, and once this lands, 📌 Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) Fixed becomes trivially unblocked: it'll only require:
- new configuration to allow declaring a preference for
image
vsmedia
(which oddly would not be possible through a newexperience_builder.settings.yml
config, only through a new config entity, because simple config cannot have module nor config dependencies) - updating
\Drupal\experience_builder\SdcPropJsonSchemaType::findFieldTypeStorage()
to respect that configuration
Those 2 things would be in an MR for that issue which I could start and then the front-end team could continue.
Alternatively, I could repurpose 📌 Introduce `hook_storable_prop_shape_alter()`, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed Fixed for that, so that #3454173 can stay focused on the bigger picture, of making that media library widget actually work (well).
- new configuration to allow declaring a preference for
- Assigned to wim leers
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Discussed #17 with @lauriii. His replies:
- Yes, but needs follow-up to devise a UX to allow existing
StaticPropSource
s using a previous field type+widget to update to the new one. - Yes
- Yes
He agreed with the 3 follow-ups proposed at the bottom of #17.
We also discussed #18, and he prefers the "alternatively" — so repurposed that 👍
P.S.: Note that this also blocks 📌 Create a component for testing form backend + frontend integration Active .
- Yes, but needs follow-up to devise a UX to allow existing
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Follow-ups:
- #19.1: 📌 [later phase] [PP-1] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed (and I realized it had a blocker itself: ✨ [later phase] PropShapes' JSON schema must match exactly with FieldType's storage format — what if you want to use another FieldType? Active ), and then the 3 at the bottom of #17:
- 📌 Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed
- 📌 [PP-2] Remove Component config entity's add/edit form Postponed
-
📌
[PP-1] Introduce `FieldTypeStorageForJsonSchemaDefinition` plugin type
Postponed
Bonus follow-ups: 📌 Component config entity should validate that the SDC actually (still) exists Active + 📌 [PP-2] Add ComponentAuditabilityTest Active .
I'll continue working on this MR on Monday. Now: time for blog posts :)
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
The MR is green and updated to account for the slightly changed 📌 Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed future 👍
- Status changed to Needs work
6 months ago 2:55pm 31 July 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Doing one final pass next.
Prioritizing this to, unblock many subsequent steps:
- front-end: 📌 Create a component for testing form backend + frontend integration Active (blocking @bnjmnm in particular)
- front-end: 📌 Introduce `hook_storable_prop_shape_alter()`, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed Fixed (blocking no one in particular, but this is critical for 🌱 Milestone 0.1.0: Experience Builder Demo Active !)
- back-end: 📌 Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed 👉 @f.mazeikis should be back on XB tomorrow, and this will be an excellent fit
- Issue was unassigned.
- Status changed to RTBC
6 months ago 8:24am 1 August 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Per #23 and a thorough self-review: going ahead and merging. I thoroughly tested this, and can confirm that this unblocks 📌 Create a component for testing form backend + frontend integration Active , because with this MR I can get:
In the MR, there's 4
@todo
s pointing to 📌 Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed , 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 👍Finally: 📌 Introduce `hook_storable_prop_shape_alter()`, use it to prefer the Media Library widget for "image" PropShape if Media Library is installed Fixed exists (and after this MR is in, it'll be a crystal-clear change in a single place, rather than hacks sprinkled all over) to avoid having to wait for 📌 [PP-1] Introduce `FieldTypeStorageForJsonSchemaDefinition` plugin type Postponed — it's similar to 📌 Create a component for testing form backend + frontend integration Active in that it'll allow the people working on XB's front-end to see not just a few in XB's component props form in the right sidebar, but pretty much the full range of field widgets. That means they can start working on making them match the design ( 🌱 UX design tracker Active ), as well as adding tests to ensure that field widgets that use
Drupal.behaviors
actually work.With that: 🚢
-
Wim Leers →
committed c5212d1a on 0.x
Issue #3461499 by Wim Leers, lauriii: Support complex SDC prop shapes:...
-
Wim Leers →
committed c5212d1a on 0.x
- Status changed to Fixed
6 months ago 8:28am 1 August 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
#24 contains the rationale for bypassing the usual review process.
This landing has actually resulted in 3 important areas getting unblocked:
- #3463583-17: Use the `all-props` component for testing form backend + frontend integration → → @bnjmnm fully unblocked, no more back-end work necessary there, he can merge when he pleases ✅ — which means the front-end contributors can now work on expanding what ✨ Contextual form values need to be integrated with Redux Active introduced to more field widgets.
- #3462709-8: Hardcode image props to use the media widget temporarily → → I've gotten the Media Library Widget to render — but it won't work well until the necessary CSS + JS assets are loaded too.
- I caught @f.mazeikis up on the last ~month of changes, and he's gotten started on 📌 Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed 👍
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Oh, and this also unblocked longer-term back-end issues in addition to #3463999:
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Automatically closed - issue fixed for 2 weeks with no activity.