Problem/Motivation

Our default page layout management is not clear

A default page layout is a layout with no condition plugin attached. Normally, a maximum of one page layout is expected in a website.

In admin UI, they are mixed with the other page layouts but:

  • they can be dragged at any position, but they disable the following page layouts, so they must always be at the bottom of the list
  • we can have many of them even if only the first one will be loaded. It may be useful for keeping some disabled ones for later
  • The "Without any Page Layout, pages are managed by Block Layout" message is misleading because it must be "Without a default Page Layout..."

In AccessControlHandler::loadCurrentPageLayout(), default page layouts must be loaded after the other ones.

Proposed resolution

Add PageLayoutInterface::isDefault() to make default page layout an "official", manageable, concept.

Split the list in admin UI, by putting the default ones at the end, and remove them from the reorder mechanism:

We keep the current "block layout" alert for now, but we may find a better way of presenting this information:

Change the behavior of AccessControlHandler::loadCurrentPageLayout()

Starting point when creating a page layout

Related to the same confusion: today a new page layout is never blank. PageLayout::getInitialSources() silently runs BuilderDataConverter::convertPage() and imports the visible blocks of the front-end theme, wrapped in the theme page shell. The user is not told, gets no choice, and does not learn that the copy is a one-time snapshot.

Proposal: ask on the creation form. Three options, differing on one axis, how much of what already exists is kept.

  • Start from your current site (recommended when the theme has blocks placed)
    Copies the blocks currently placed in the theme into their matching regions, still rendered by the theme's own page template, so pages look the same as they do now. It is a one-time copy: later changes in Block Layout will not show up here. Replace each region's contents with components as you go, then remove the theme page shell to take full control of the page.
  • Minimal Drupal page
    Places only what a Drupal page needs to keep working: page title, status messages, help, tabs, primary actions, breadcrumbs and the main content. The theme's page template is not used, so the page is unstyled until layout components are added.
  • Blank
    Nothing at all, for building the page entirely from components.

The import is offered only while the site has no built page layout, and never again. It is an adoption ramp, not a feature. Every layout after the first gets Minimal (preselected) and Blank only, plus one line: to start from a layout you already built, duplicate it from the page layouts list. Re-importing into a conditional layout would carry a second copy of the whole site's block layout and reintroduce the theme page shell the user is working to remove.

Minimal must not include the theme page shell. Otherwise Minimal and the import differ only by which blocks sit inside the shell, and "I own the page" exists only as Blank. Keeping the shell out is what makes the three options a gradient.

Help text belongs on the creation form as the description of each option. Not a wizard, not a modal, not a tooltip. That is the one moment the user is thinking about this question.

The theme page shell is a seed, not a component

Consequence of the above, and it decides how the shell behaves elsewhere:

  • Not addable from the component or block library. It arrives as a seed fromr way, which is what stops it becoming a permanent parallel regime.
  • One per layout. Two shells render page.html.twig twice.
  • Named for what it is, "Theme page shell", with a description saying its regions are rendered by the theme's page template. "Page layout (from active theme)" is confusing when everything here is a page layout.
  • Shown as a boundary rather than an ordinary component in Canvas, Wireframe and Navigator. A distinct label treatment is enough.
  • While it is present, one visible line stating the exit: move a region's cohe theme stops rendering that part. That sentence is the whole migration path and is currently written nowhere.

Relation to the default layout work in this issue

Both come from the same gap: the UI never says which layout catches everything, nor what a new layout starts from. Alongside isDefault() and the list separation, two cheap additions on the list builder:

  • Warn on unreachable layouts: any enabled, non-empty layout ordered below an enabled, non-empty, condition-less one can never match. The failure is invisible today and produces "my layout does nothing".
  • When no catch-all exists, say the remaining pages are still handled by Block Layout, and frame it as work remaining rather than as a feature.
CommentFileSizeAuthor
#2 db-page-layout-list-2.png130.11 KBpdureau
db-page-layout-list.png124.27 KBpdureau

Issue fork display_builder-3614540

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

pdureau created an issue. See original summary.

pdureau’s picture

pdureau commented
Issue summary: View changes
StatusFileSize
new db-page-layout-list-2.png130.11 KB

pdureau opened merge request !326

pdureau’s picture

pdureau commented
Status: Active » Needs work

Started.

Done:

  • Entity list in admin UI
  • AccessControlHandler::loadCurrentPageLayout() update

TODO:

  • Tests?
  • Distinct entity form when no existing default page layout?
mogtofu33’s picture

mogtofu33 commented
Assigned: pdureau » mogtofu33
mogtofu33’s picture

mogtofu33 commented
Parent issue: » #3612556: PageLayoutSource is sometimes not rendered
mogtofu33’s picture

mogtofu33 commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Needs work » Needs review

First version for review with starting point strategy and explanation for user, I think just the default page rows in the table need a fix.

In needs review for blocking feedback.

pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33
Status: Needs review » Needs work

The addition of the default page layout creation page is cool. I am sure users will feel more guided and comfortable.

First review:

StartingPoint service

The logic of StartingPoint::isImportAvailable() is opaque. As an user, why would I not want to import from the current theme's layout only if there is at least one Page Layout config entity with empty sources ? That's so specific. The full method can be removed in my opinion.

The full display_builder_page_layout.starting_point service is questionable. The only other public method is getSources() which is doing the work of the existing \Drupal\display_builder_page_layout\Plugin\display_builder\Buildable\PageLayout::getInitialSources(). We can make the economy of a full service here and keep display_builder_page_layout consistent with other sub-modules.

DefaultPageLayoutAccess service

Same for the other service introduced in the MR:

  display_builder_page_layout.default_page_layout_access:
    class: Drupal\display_builder_page_layout\Access\DefaultPageLayoutAccess
    tags:
      - { name: access_check, applies_to: _page_layout_no_default }

Is used only in this route with a custom requirement key:

entity.page_layout.add_default_form:
  requirements:
    _page_layout_no_default: 'TRUE'

We can also make the economy of a full service here, and keep our code familiar, by using the standard _custom_access requirement key: https://www.drupal.org/docs/8/api/routing-system/access-checking-on-rout...

entity.page_layout.add_default_form:
  requirements:
    _custom_access: Drupal\display_builder_page_layout\Access\DefaultPageLayoutAccess

"The default page layout %label is disabled" message

If we have many "default" page layouts (many page layout entities with no condition plugin attached), we check only the first to display the "The default page layout %label is disabled, so pages matched by no layout below are still built by Block Layout...." message.

I know we are limiting to one the number of default page layout entities we can have thanks to DefaultPageLayoutAccess and PageLayoutForm::validateForm(), but:

  • it is an UI only restrictions, people creating and managing Page Layout entities from code are still able (and that's a good thing ;) let's not prevent innovative use of our API's)
  • there are currently Display Builder users, sometimes in prod, with many default page layouts and the one they use may not be the first

So, instead of checking the first, let's check all and print the message only if none are valid (valid = enabled && not empty).

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Needs work » Needs review

I don't like the _custom_access because it is Drupal specific, but I can switch I do not really care. Updated.

Handled multiple defaults for messages.

Agree to move the StartingPoint service, but arguments are a bit weak, "questionable" is a verdict, not a criteria, what rule it breaks for being a service?
"economy" is weak as well, Drupal container is compiled and services are lazily instantiated. So this one costs nothing at runtime. Core ships well with a lot of services. Nobody scrutinize the number of services.
In the same file we have lazy builder_data_converter which is kind of the same shape with a single caller.
But other remarks on duplication were more valid.

pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33
Status: Needs review » Reviewed & tested by the community

Drupal container is compiled and services are lazily instantiated. So this one costs nothing at runtime. Core ships well with a lot of services. Nobody scrutinize the number of services.

OK.

Handled multiple defaults for messages.

Tested, it works well, thanks.

StartingPoint::isImportAvailable() has been replaced by StartingPoint::isDefaultLayout(), it is better because the duality default layout / other layouts is more guessable and understandable by the users.

I still believe it is a bit weird this option is limited to default layout (so it can be used only one time) but I move the ticket to RTBC because everything else is great, and no need to prevent the merge only for this last point.

  • mogtofu33 committed 63ba2349 on 1.0.x authored by pdureau 
    task: #3614540 Clarify default page layout

By: pdureau
By: mogtofu33
mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » Unassigned
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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