Split 1 PageTemplate config entity into N PageRegion config entities

Does it worsen DX of config deployments (e.g., doing a manual code review across per-region YAML files vs within a single YAML file)?

My gut feeling is that this isn't significantly worsened at least in the case of a single page template. The impact would be more in the case of multiple page templates. It seems like a trade-off that could be accepted.

Or on the other hand, would it add useful capabilities like creating variants for some regions while leaving other regions shared across page variants?

This could actually be a benefit because for example the header and footer might remain the same on every page template but the content top might need to change for some pages.

I'm wondering if based on this there should be a page template config which can specify a parent page template and is allowed to depend on specific region configurations that would override the parent?

🤯 I didn't see this coming for sure. #2 is accurate — that's how @lauriii always described it.

I definitely agree with the cons, especially the variants one. Although @lauriii makes a strong observation in #3.

I'll think about this some more. I urgently need to review some things, leaving not enough time to think through all consequences.

AFAICT this would make #3495598: [PP-1] Don't store page template model data in auto-save for an entity obsolete too, and hence we could add it to the "avoid" list?

I don't think this makes #3495598: [PP-1] Don't store page template model data in auto-save for an entity irrelevant, we still store the entire model against all autosaved entities - the aim of that issue is only store the parts of the model that are relevant to each entity.

You're right, I misread & misunderstood!

Remember: the future GlobalPageTemplate config entity 👻

#3478537: Introduce an XB `PageTemplate` config entity introduced the PageTemplate config entity (config schema type: type: experience_builder.page_template.*:). It comes with this comment:

# Otherwise, an Experience Builder Page Template can be created per installed theme, and stores component trees for each
# region in a theme's `page.html.twig`.

That "otherwise" refers to the preceding config schema type: a placeholder for a future piece of functionality that is very closely related: the global page template config entity.

That looks like so:

# At most a single entity of this type is allowed to exist. If it exists, it *replaces* the theme's `page.html.twig`.
experience_builder.global_page_template.*:
  type: config_entity
  constraints:
    FullyValidatable: ~
  mapping:
    # A single component tree.
    component_tree:
      # @todo This MUST contain a single XB Component that points to the title block, main content block and messages block
      # @see \Drupal\Core\Block\MainContentBlockPluginInterface
      # @see \Drupal\Core\Block\TitleBlockPluginInterface
      # @see \Drupal\Core\Block\MessagesBlockPluginInterface
      # @todo Blocked on Blocks-as-XB-Components: https://www.drupal.org/project/experience_builder/issues/3475584
      type: experience_builder.component_tree
      label: 'The component tree that represents the default page'

How would you imagine that to work in the new world proposed by this issue? 🤔 AFAICT, that'd remain unchanged, because it completely ignores the "default/active theme"'s regions.

(Sadly, I can't find any "global page template" details: it's based on discussing this with @laurii IIRC, and is about the "global template" vs the "page template" as captured by https://docs.google.com/spreadsheets/d/1OpETAzprh6DWjpTsZG55LWgldWV_D8jNe9AM73jNaZo/edit?gid=1721130122#gid=1721130122&range=I29">requirements 18 vs 19.)

+1 if we're on the same page

If y'all agree that splitting PageTemplate into N PageRegion config entities does still leave the GlobalPageTemplate config entity unchanged … then I'm +1 to this proposal, and I do think that it'll make things simpler 😊

Bonus! 🚀

In fact, a nice consequence will be that if you try to uninstall a module that provides some SDC or Block, you'd get a more precise overview in the "oops, can't do that, because these config entities depend on it" UI: it'd tell you exactly which regions it's being used in, and in the future even which variants! 🥳

To add to #7: that'd also mean that the content region would either:

• never get a PageRegion content entity, because that region is hardcoded (in code) to contain the "main content" block
• always get it, but we'd never expose it and it'd be a de facto hardcoded config entity

Related: #3502372: Hide main page content block from XB

Given @lauriii specifically opened #3502372, requesting him to approve #7 + #8.

I'm confused about what concept GlobalPageTemplate represents. […] should be renamed to PageTemplate.

I think that entire comment captures much more clearly than ever before what @lauriii intended all along! 😄

But it's thanks to the

• 1 PageTemplate config entity per theme containing data for every region → N PageRegion config entities per theme, each containing data for 1 region
• 1 GlobalPageTemplate → 1 PageTemplate

rename that you were able to describe it so clearly! 🤩 👏👏

I'd like @lauriii to explicitly confirm to make sure this matches what he envisioned 😊

P.S.: happy to do this one, I think I'm well-positioned to do this system-wide refactor. 🤓

#10 and #13 seems like a really solid plan for this. Thank you @effulgentsia and @wim leers 👏

My original thinking last spring was that we would have a single config entity which would store the "page template" meaning the regions available and "block placement" meaning what's inside those regions. We could implement this in phases so that defining the regions is optional and would rely on the theme (to allow integration with existing themes). We've learned since then that this didn't consider all of the factors yet and therefore it was insufficient. I'm +1 to splitting the individual component trees for regions to separate config entities

In my head, global page template is the idea that there would be one template which is the default template (page.html.twig). You could then specify overrides of that for specific parts of the site, e.g. specific content type (page--node--article.html.twig). The latter would be just a regular page template because it's only applied to a specific section of the site. Once we allow multiple page template, it might be clearer to call the global page template the default template at least in code.

I find the last paragraph of #14 quite confusing, but all of that is out of scope here anyway, so let's save that discussion for later, when we actually work on it.

It doesn't need to be me who implements this, so unassigning.

Adding to #8: #3505872: Page title block should not be required landed too, so the "validation across the component trees for all regions in a given theme" just became simpler.

To recap, there are 3 special kinds of blocks: "main content", "page title" and "messages".

1. as of #3505872: Page title block should not be required, the page title no longer gets special treatment — thanks to @lauriii's response to #3505872-5: Page title block should not be required.
2. per #8, the "main content" one no longer needs any special treatment either, at least not at the validation level. Because we'll hardcode that block being the sole one in the content region.
   (This also came up at #3502371-5: Make "Page title block" work A) also outside regions, B) on the only routes XB currently supports: content entity routes and #3502371-6: Make "Page title block" work A) also outside regions, B) on the only routes XB currently supports: content entity routes.)
3. that leaves only "messages"! I personally think it merits getting almost the same treatment as the page title one, with one difference: if \Drupal\experience_builder\Plugin\DisplayVariant\PageTemplateDisplayVariant didn't encounter >=1 such block during rendering, then XB's page variant should just place it at the top of the content region automatically, which is exactly what BlockPageVariant does:
   

       // If no block displays status messages, still render them.
       if (!$messages_block_displayed) {
         $build['content']['messages'] = [
           '#weight' => -1000,
           '#type' => 'status_messages',
           '#include_fallback' => TRUE,
         ];
       }
   

If @lauriii agrees with that, then we'll have eliminated all of the validation challenges in this MR! It'll allow this MR to delete \Drupal\experience_builder\Plugin\Validation\Constraint\ComponentTreeMeetsRequirementsConstraint::$nested and related code.

Agreed that it would be great to place take care of placing the message. I like the proposal to place the message as a fallback on top of the content region. 👍

FYI: this blocks #3502769, see #3502769-4: Auto-saved PageRegions may be invalid, but MUST NOT cause XB's previews to fail..

Moved this along a bit further, fixed all phpcs/phpstan issues and fixed some of the tests. More tests to fix and Wim's review points to address still.

Both @longwave and @f.mazeikis are out today, I'll finish this.

This is now down to a single last failure that's tricky to debug.

Critical bugs found:

1. https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...
2. https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...
3. #17 was not yet addressed: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...
4. lost test coverage: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... — fixing this also fixed #3502372 as predicted in #3502372-2: Hide main page content block from XB

Interesting non-critical bugs found:

• https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...

Now the word "template" in the XB codebase never refers to PageTemplate anymore.

Out-of-scope

1. #3507334, for which I rolled a patch over here already: #3507334-2: Deleting/updating a config entity and deleting a content entity should clear its values from the auto-save store
2. The pre-existing #3502902: Only auto-save content entities/PageRegion config entities when there are actual changes: simply previewing incorrectly causes them to appear in "Review x changes" is also happening here now, but fixing that is out of scope here:
   

global-regions.cy.js is green locally.

I've got it! 🥳 First green block-form.cy.js CI run.

Wow, what a nightmare this was to debug — treating client-side data as unvalidated blobs that must be respected sure results in mind-bending debugging, because the bug/mistake can happen much earlier than the failing test! 😳🧟‍♀️

Whew, it's finally in!

Didn't want to drag this >1000 LoC heisenbug-esque monster forward any longer. And didn't want to subject anybody to the painful review, although both @longwave and @larowlan already did. And, of course, I reviewed @f.mazeikis' original MR.

99% of the pain was in the auto-saving parts (see #25 for why — and OpenAPI doesn't help here at all because the client is supposed to be able to send arbitrary blobs!). The test failures when I started working on this were misleading/largely irrelevant, because the foundations were not yet working (see #23, "critical bugs").

The goal is always to first make the config entity iron-clad and thoroughly validated. Then build upon it. (And in this particular case, debugging was surprisingly painful.)

Quoting from #17:

If @lauriii agrees with that, then we'll have eliminated all of the validation challenges in this MR! It'll allow this MR to delete \Drupal\experience_builder\Plugin\Validation\Constraint\ComponentTreeMeetsRequirementsConstraint::$nested and related code.

Extracted that into a simple follow-up: #3507549: Remove unused ComponentTreeMeetsRequirementsConstraint::$nested.

More follow-ups tomorrow.

After updating to 0.x and reinstalling seeing #3507631: Page mangled and warning from block components when enabling for global template

All out-of-scope follow-ups, as promised in #27, for things I ran into while working on this MR:

• #3507549: Remove unused ComponentTreeMeetsRequirementsConstraint::$nested
• #3507704: PHPStan conditional return type on ComponentTreeItem::get() allows removing a lot of assertion noise
• #3507749: Clarify default 'resolved' vs 'source' value logic in GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo()
• #3507716: Simplify ClientServerConversionTrait::clientLayoutToServerTree()