- Issue created by @phenaproxima
- πΊπΈUnited States phenaproxima Massachusetts
Self-assigning to start on this.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Left some comments, looking slick
- πΊπΈUnited States phenaproxima Massachusetts
OK, this is pretty much where I want it. After discussion with Lee and Matt, I don't think this needs anything more than kernel test coverage. We don't need to test actual rendering yet, since there's nowhere for the template to store the component tree (yet). We just need to ensure that the correct template is chosen in specific situations.
- πΊπΈUnited States phenaproxima Massachusetts
I think I've fixed all the feedback so far.
After some discussion with @lauriii, the scope here can be reduced further. Wim pointed out that the "rendering algorithm" came sorta out of nowhere, so I asked @lauriii about it and he came up with the rendering flow he would actually like XB to implement. It's not in scope here, so I'm punting that to a separate issue that I have yet to open.
So indeed, this issue is just about adding the
ContentTemplate
config entity (the class name was blessed by @lauriii), but not actually using it anywhere. That's a reasonable first step. - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Thanks for extracting all the view builder-related things into β¨ Content templates, part 2: Using the templates for rendering Active β much clearer scope now!π And agreed with excluding exposed slots from this initial MR π
Found a few data integrity issues:
- some config entity properties should be immutable but aren't yet
- missing dependencies that should be calculated but aren't yet (but for which the logic already exist elsewhere in the XB codebase!)
- missing validation for the stored component tree (for which all the validation logic already exists)
- missing follow-up to validate that
ContentTemplate
config entities can only be created for eligible content entity types+bundles (this follow-up should itself be postponed on π Allow XB to be used on all node types Active , but MUST be tagged )
The new
ViewModeExistsConstraintValidator
really needs a core issue to fix it upstream; and the current name unfortunately suggests it's fine to keep around forever. We should rename it and point to the core issue to eventually remove it from XB.It's fine to defer some of the detailed test coverage to a follow-up, but with so many of the necessary validation foundations already existing in HEAD and this copying-and-expanding my PoC MR from π [PoC] Introduce a `ContentTypeTemplate` config entity Active , I think it's reasonable to require this to be solid from the very start.
Otherwise, I promise that the next step ( β¨ Content templates, part 2: Using the templates for rendering Active ) will immediately run into problems when it writes its test coverage π
- πΊπΈUnited States phenaproxima Massachusetts
Opened all relevant follow-ups and linked them in the code.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Thanks for opening all those follow-ups! This is looking fantastic. Doing a final review pass.
FYI: I'm really scrutinizing this to make all subsequent issues A) simpler (because more actionable config schema errors will guide you!), B) hence need less of my attention π
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
I:
- enabled config schema checking for the new kernel test: https://git.drupalcode.org/project/experience_builder/-/merge_requests/8... β automatically revealed bugs in the test coverage π₯³
- Fixing that revealed that actually the depended upon JS Component did not exist.
- β¦ and then a whole bunch more things, too many to enumerate, and in doing so I ended up realizing
PageRegion(ValidationTest)
was worse than this MR in some ways, so I made it catch up!
Also: detailed guidance/pointers at:
- #3481720-2: Tighten validation: only allow StaticPropSource in XB fields + PageTemplate, DynamicPropSource in ContentTypeTemplate β
- #3518248-6: Content templates, part 4 (boss battle): create a UI for editing templates β
Epic work here!
-
wim leers β
committed 989b6996 on 0.x authored by
phenaproxima β
Issue #3517741 by phenaproxima, wim leers, larowlan, mglaman: Content...
-
wim leers β
committed 989b6996 on 0.x authored by
phenaproxima β
Automatically closed - issue fixed for 2 weeks with no activity.