Overview

This is one of the config entity types XB needs — see #3444424: [META] Configuration management: define needed config entity types.

For version 0.2, XB needs to be able to modify the page template (aka page.html.twig).

(See #3455753: Milestone 0.2.0: Early preview.)

Product requirement 19. Modify the page template (i.e. global sections) states:

As a builder, I want to modify the page template (i.e. page.html.twig). When I modify the page template, I can place components globally to global regions like navigation, header, footer, etc.

Designs + UX still needed. This is merely laying the technical groundwork inferred from that product requirement, and clarified by talking to @lauriii and @effulgentsia.

There are two distinct needs:

  1. Short-term/for existing sites and themes: ability to use Experience Builder to populate a theme's existing regions (which are essentially "slots" in the page.html.twig template).
  2. Long-term: ability to not use a theme's page.html.twig, and just craft that from scratch

Out-of-scope, follow-up issue needed:

Proposed resolution

See the updated docs/config-management.md.

In a nutshell:

  • new strictly validated PageTemplateconfig entity type, with thorough config validation (and config dependency) test coverage in PageTemplateValidationTest() — which reuses the pre-existing validation logic for the type: experience_builder.component_tree config schema type
  • that includes a new validation constraint that ensures the presence of certain essential page-level components (blocked on #3475584: Add support for Blocks as Components but proven to work in this MR) as well as ensure the absence of prop sources that cannot reliably work at the page level dynamic prop sources) — this validation constraint will also be usable in #3479643: Introduce a `Pattern` config entity, which this hence blocks
  • new \Drupal\experience_builder\Plugin\DisplayVariant\PageTemplateDisplayVariant page display variant plugin with thorough functional test coverage in \Drupal\Tests\experience_builder\Functional\PageTemplateVariantTest

👆 This will allow reliable use of config management (#3444424: [META] Configuration management: define needed config entity types).

User interface changes

None.

Issue fork experience_builder-3478537

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:

About issue forks

Comments

wim leers created an issue. See original summary.

wim leers opened merge request !347

johnwebdev’s picture

johnwebdev commented

I have a question that I’ve been thinking about around this, maybe a bit early to answer

Let’s say I have a header region, and I place a Hero component in that region. The Hero component requires an background image (Media entity)

How will that work? Let’s say I build the page template locally and then push the config entity to production, there is no guarantee that the media with that ID exists so the default will be broken image when you create a page.

Thinking loudly… Should page templates e.g be allowed to save “incomplete” components, or do we need some placeholder feature generic, or specifically for media?

wim leers credited effulgentsia.

wim leers credited lauriii.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

This is progressing nicely. An incomplete PageTemplate config entity now fails as expected:

1) Drupal\Tests\experience_builder\Kernel\Config\PageTemplateValidationTest::testEntityIsValid
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 []
+Array &0 [
+    'component_trees' => Array &1 [
+        0 => 'Configuration for the region "<em class="placeholder">sidebar_first</em>" (<em class="placeholder">sidebar_first</em>) is missing.',
+        1 => 'Configuration for the region "<em class="placeholder">sidebar_second</em>" (<em class="placeholder">sidebar_second</em>) is missing.',
+        2 => 'Configuration for the region "<em class="placeholder">content</em>" (<em class="placeholder">content</em>) is missing.',
+        3 => 'Configuration for the region "<em class="placeholder">header</em>" (<em class="placeholder">header</em>) is missing.',
+        4 => 'Configuration for the region "<em class="placeholder">primary_menu</em>" (<em class="placeholder">primary_menu</em>) is missing.',
+        5 => 'Configuration for the region "<em class="placeholder">secondary_menu</em>" (<em class="placeholder">secondary_menu</em>) is missing.',
+        6 => 'Configuration for the region "<em class="placeholder">footer</em>" (<em class="placeholder">footer</em>) is missing.',
+        7 => 'Configuration for the region "<em class="placeholder">highlighted</em>" (<em class="placeholder">highlighted</em>) is missing.',
+        8 => 'Configuration for the region "<em class="placeholder">help</em>" (<em class="placeholder">help</em>) is missing.',
+        9 => 'Configuration for the region "<em class="placeholder">page_top</em>" (<em class="placeholder">page_top</em>) is missing.',
+        10 => 'Configuration for the region "<em class="placeholder">page_bottom</em>" (<em class="placeholder">page_bottom</em>) is missing.',
+        11 => 'Configuration for the region "<em class="placeholder">breadcrumb</em>" (<em class="placeholder">breadcrumb</em>) is missing.',
+    ],
+]

(Also crediting Lauri & Alex.)

catch’s picture

catch
he/him
Primary language English
commented

How will that work? Let’s say I build the page template locally and then push the config entity to production, there is no guarantee that the media with that ID exists so the default will be broken image when you create a page.

The way this works with similar things now is you would have to do this:

1. Create the media entity on production
2. Sync the production database locally
3. Set up the layout (or whatever config entity referencing the content entity).
4. Deploy the config and everything works

This works but is of course very clunky. Feels like a separate issue since it's more of a general problem with config entities that reference content entities rather than XB specific, although agreed it's going to be a common thing people want to do.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#3: @catch is right in #7 and is also right that this is out of scope here, but merits its own issue. For now, let's discuss this on the meta, because this is a challenge affecting multiple of XB's config entity types. See prior discussion at #3444424-4: [META] Configuration management: define needed config entity types from >4 months ago, where I link to a PoC that we built >5 months ago that shows a viable way to fix that.

Let's not discuss that further here 🙏

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Active » Needs review
Issue tags: -Needs documentation

Will continue on Monday, but this is ready for an initial high-level review. 👍

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Status: Needs review » Needs work

Continuing…

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » longwave
Issue summary: View changes
Priority: Major » Critical
Status: Needs work » Needs review
Issue tags: -Needs issue summary update +blocker
Parent issue: » #3450586: [META] Back-end Kanban issue tracker

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

A few questions but mostly nits, I think the overall approach is sound.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Assigned: longwave » wim leers
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » lauriii
Status: Needs work » Needs review
Related issues: -#3450586: [META] Back-end Kanban issue tracker +#3475584: Add support for Blocks as Components

@lauriii A review from @longwave surfaced two important questions, and I answered both by adding explicit "warning, future stuff" docs for both. Can you confirm the accuracy of these 2 small paragraphs? 🙏

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Assigned: lauriii » Unassigned

#15: That looks good 👍 Those are limitations I'm aware of for the current state of XB.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » longwave

Thanks @lauriii! Incorporated your suggested refinement too 👍

All feedback from @longwave has now been addressed, back to him for review.

catch’s picture

catch
he/him
Primary language English
commented

I did a (fairly quick/cursory) review of this and couldn't see any obvious problems, just one question in the MR.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Good question/suggestion, @catch! Responded :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Related issues: +#3479982: HTTP API to read+write PageTemplate and Pattern config entities

The HTTP API follow-up exists: #3479982: HTTP API to read+write PageTemplate and Pattern config entities.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Merged in upstream and did a trivial search-and-replace to compensate for #3480193: Replace plus with dot in Component config entity IDs having landed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

FYI: #3479982: HTTP API to read+write PageTemplate and Pattern config entities is making good progress and will allow a (client-side) UI to be built, allowing to reuse significant pieces of the existing UX :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3481188: [later phase] Gracefully handle deleted regions in PageTemplate config entities"

Follow-up created per @lauriii's feedback: #3481188: [later phase] Gracefully handle deleted regions in PageTemplate config entities".

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Assigned: longwave » wim leers
Status: Needs review » Needs work

Again only another round of tiny nits, I still think this is a solid base for the desired functionality here.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Reviewed & tested by the community

Thanks, this looks great now, and unblocks future work. Let's ship it!

  • wim leers committed f1e3abde on 0.x 
    Issue #3478537 by wim leers, longwave, lauriii, catch, effulgentsia:...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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