Problem/Motivation

A new feature has been added to beta9 without being discussed and reviewed: #3632193: Add a homepage page layout

Site builders ask for an easy way to build the homepage with a page layout.

I have never heard of homepage being a specific requirement from users and curious site builder. Ever. Was it confused with "Landing page" ? Because it remembers me the discussions about docs/build-a-homepage.md.

What people want, badly, is a landing page feature, where landing pages are content entities :

  • authored in production by content contributors
  • with only 3 base fields: title, UI Patterns source tree, and a path
  • with display Builder as the only edit-form. That's important, title and path must be edited there.
  • with only a single display: full. Which is not buildable.

A bit like Canvas' Page entity type. We shared some doubts about such feature, that why we didn't rush it. I will create a ticket for post 1.0.0 to discuss them.

Moreover, what was shipped last week has a few issues:

  • how come the homepage is special in a page layout point of view? Like a splash page ? Seems niche and not worthy of an hardcoded mechanism.
  • homepage is pinned at the top, mimicking the behaviour of default page layout, but it misses the point. We pinned default page layout on bottom to not be triggered before other page layouts (it is only the default one, must be checked at the end), while giving a visual clue of this rule. It doesn't make sense for homepage, which has conditions like the others.
  • It makes local actions of /admin/structure/page-layout busy. What's next? An hardcoded user login page?
  • This apparently: #3632199: One default and one homepage per theme, and a Theme column in the display lists

Proposed resolution

Just revert.

CommentFileSizeAuthor
#6 3633009 after, add.png301.32 KBcsakiistvan
#6 3633009 after.png590.25 KBcsakiistvan
#6 3633009 before.png529.75 KBcsakiistvan
#3 home-after.png26.83 KBpdureau
#3 home-before.png26.87 KBpdureau

Issue fork display_builder-3633009

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 opened merge request !513

pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new home-before.png26.87 KB
new home-after.png26.83 KB

MR: https://git.drupalcode.org/project/display_builder/-/merge_requests/513

The commit has been cleanly reverted. If an homepage exists, it is kept and moved where it belongs:

1.0.x This MR

pdureau’s picture

pdureau commented

A bit like Canvas' Page entity type. We shared some doubts about such feature, that why we didn't rush it. I will create a ticket for post 1.0.0 to discuss them.

Ticket created: #3633010: Add landing pages

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: Unassigned » csakiistvan
csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: csakiistvan » Unassigned
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 3633009 before.png529.75 KB
new 3633009 after.png590.25 KB
new 3633009 after, add.png301.32 KB

✅ Tested and works — created a homepage layout on 1.0.x via "Create the homepage", then applied MR !513, rebuilt caches and reloaded /admin/structure/page-layout: the local action is gone, /admin/structure/page-layout/add-homepage returns 404, and the existing homepage layout is kept, no longer pinned, and its builder still opens. The revert is clean for sites that already created a homepage.

pdureau’s picture

pdureau commented
Assigned: Unassigned » mogtofu33
mogtofu33’s picture

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

You are right on process: I merged #3632193: Add a homepage page layout the same day without review, so let's settle it here.

The homepage is not like other conditional layouts, though: with the front page on /node/1, a layout on /node/* takes it unless the homepage is checked first. Pinned on top, it gives the same visual clue as the default at the bottom.

#3633010: Add landing pages is content for editors after 1.0, so it does not cover this need for site builders.

If the new kind is the problem, I suggest keeping only a shortcut: the action prefills Pages with <front> and saves on top of the list, with no isHomepage(), no pinned row and no refused condition.

In the meantime the revert would need a post_update because of the default weight 0, this would make a /node/* layout silently take the front.

pdureau’s picture

pdureau commented

The homepage is not like other conditional layouts, though: with the front page on /node/1, a layout on /node/* takes it unless the homepage is checked first. Pinned on top, it gives the same visual clue as the default at the bottom.

Interesting. I will have a look on this.

pdureau’s picture

pdureau commented
Status: Active » Needs work

Test of the MR with /node/1 as homepage in /admin/config/system/site-information

And 2 page layouts:

  • One for /node/* with a primary navbar
  • One for <front> with a secondary navbar

Results:

On /node/1 On /
/node/* before <front> ✅ Primary navbar (/node/*) ✅ Primary navbar (/node/*)
<front>before /node/* ⚠️ Secondary navbar ( <front>) ✅ Secondary navbar ( <front>)

There is something weird to clarify, indeed.

  • The front page is an alias of /node/1 (there is no other existence of the front page which would not be /node/1 ), so it is normal the front page takes the layout of /node/* if /node/* comes first.
  • But the /node/1 is not an alias of the front page (/node/1 also exists without being the front page), so why would it takes the layout of <front> if <front> comes first?

This is how Core's works:

  • request_path condition plugin: A block shown only on <front> also appears on /node/1.
  • PathMatcher::isFrontPage() is TRUE on /node/1.

I will investigate what can be proposed.

pdureau’s picture

pdureau commented
Status: Needs work » Needs review

I have rebased and added a commit to "fix" the behaviour of request_path condition plugin for our usage (without altering the usage of the plugin elsewhere):

On /node/1 On /
/node/* before <front> ✅ Primary navbar (/node/*) ✅ Primary navbar (/node/*)
<front>before /node/* ✅ Primary navbar (/node/*) ✅ Secondary navbar ( <front>)

I believe it is more sensible and targeted approach of the issue, but I have to admit am not 100% comfortable with having in Display Builder a different behaviour for a well knownn Core plugin.

Kept assigned to me because I am evaluating the creation of a related Core issue.

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: pdureau » csakiistvan
csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented

✅ Tested and works — rebased MR !513 locally onto current 1.0.x, set the front page to /node/1, created a <front> layout before a /node/* layout, then applied 87ceff67, rebuilt caches and loaded / and /node/1: before, both got the <front> layout; after, / gets <front> and /node/1 gets /node/*, and with /node/* first both get /node/*. This matches your latest results table.

Strategy
  • Weight only: keep core semantics, document putting <front> above /node/*.
  • Runtime override (this MR): fixes the trap, but Pages now differs from block visibility, negation included, with no hint in the UI.
  • Dedicated "Front page URL" condition: explicit, leaves request_path untouched, costs one small plugin.

I'd ship 1.0 with the plain revert plus the ordering doc note, and add the dedicated condition later if still needed.

pdureau’s picture

pdureau commented

Hi István,

Thanks for the review. I am happy to see the fix is also working for you.

fixes the trap, but Pages now differs from block visibility, negation included, with no hint in the UI.

However, alongside the review, it will be very valuable to have an opinion about the strategy here. What do you think about this condition plugins being altering this way?

There is a hint in the UI 😉 : "Unlike in block visibility, is the front page URL only, not the page it points to."

I have created the Core issue by the way: #3635229: Challenge frontpage detection in request_path condition plugin

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: csakiistvan » Unassigned

@pdureau You're right, I missed the description hint, sorry about that. Given that, my concern is mostly gone.

On the strategy: altering request_path at runtime is acceptable to me as long as it is scoped to Display Builder's Pages and treated as temporary. With #3635229 open, I'd add a @todo referencing it next to the override, so it can be dropped once core detects the front page itself. That seems better than introducing a dedicated condition plugin we'd have to deprecate later.

So +1 for shipping this MR as is (plus the @todo).

pdureau’s picture

pdureau commented
Assigned: Unassigned » pdureau

Thanks István, I feel more confident now.

I will rebase the MR and send to Jean as RTBC.

pdureau’s picture

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

  • mogtofu33 committed f046e69e on 1.0.x authored by pdureau 
    task: #3633009 Remove homepage page layout

By: pdureau
By: csakiistvan...
mogtofu33’s picture

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

Looking good, thanks.

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.