Problem/Motivation

The instances, page layout and Views lists behave differently:

  • Instance operations added by other modules (for example dev tools Import and Export) only show on the instances list.
  • No list tells whether a display is published.
  • Columns differ: Last log on two lists, no Updated date on page layouts, and the Views list shows the profile ID instead of its label.
  • The Views list is not sorted, its pager never works, and it offers no Edit operation.
  • The Instances panel misrepresents empty displays and unpublished drafts.

Each list page also carries its own description, rendered in the page body instead of Drupal's help region, and the documentation assumes Standard creates the Article content type, which it no longer does since Drupal 11.4 (#3159848: [Policy] Always install Drupal with Standard on the UI, pared down of use case specific elements (content types, node listing, commenting, theme)).

Proposed resolution

Lists

  • The page layout and Views lists call hook_entity_operation_alter() with the row's instance, so instance operations show on every list.
  • A Publication column on all three lists: Empty, Not published, Unpublished changes or Published. Drafts and published sources are compared without their node IDs.
  • Status badges use default_admin's gin-status classes, plus db-* classes.
  • Same columns everywhere: Profile label, Publication, Updated. Last log is removed.
  • The Views list is sorted by Updated, newest first, with 50 rows per page. The page layout list stays unsorted and unpaged: its order is the matching priority.
  • The Views list offers the operations of an entity list, in weight order: Build, Edit (the view display), Change profile (the display's Display Builder option form) and Delete.
  • The Instances panel shows no indicator for an empty display, and "Not published yet" for an unpublished draft.
  • New helpers in DisplayBuilderHelpers: hasUnpublishedChanges(), publicationBadge(), statusBadge().

Note: Claro fallback is ok, only badge colors (published / empty...) are not in Claro, which for me is ok as it's a deprecated admin theme. Gin 5 is good as it rely on the same classes as default_admin.

Help and documentation

  • Page descriptions move to hook_help(), so they render in Drupal's help region: instances, profiles, pattern presets, page layouts and the Views list, plus the profile add, edit and panel settings forms and the page layout add, edit and duplicate forms.
  • Each help text links to its own documentation page instead of the documentation home.
  • New tutorial, Build a homepage: a content type, its display, the front page setting, and a page layout with the <front> condition, from a Drupal install without the recipes.
  • Build your first display explains that Standard no longer creates Article, and adds it with drush recipe core/recipes/article_content_type.

Common lists

Remaining tasks

  • Follow-up: Instance::isPublishedPresent() compares raw hashes, node IDs included, while the lists compare content without node IDs, so the builder and the lists can disagree:
    • On an empty, never published display, it returns FALSE: Publish is enabled with nothing to publish, and Save status shows "Saved".
    • On a published display whose published sources carry fewer node IDs than the draft, it returns FALSE although nothing changed: Publish is enabled, Restore is offered, and Save status shows "Saved" instead of "Published".

    Using DisplayBuilderHelpers::hasUnpublishedChanges() there would align both.

  • Follow-up: #3623275: Replace the instances admin list with a View

User interface changes

New Publication column, same columns on all lists, a sortable and paged Views list with Edit and Change profile operations, and page help in Drupal's help region.

API changes

New static helpers in DisplayBuilderHelpers. ViewsManagementController takes two more autowired constructor arguments.

Data model changes

None.

CommentFileSizeAuthor
#6 3623326_lists.jpg367.78 KBmogtofu33
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:

Comments

mogtofu33 created an issue. See original summary.

mogtofu33’s picture

Assigned: Unassigned » pdureau
Status: Needs work » Needs review
mogtofu33’s picture

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

Sorry, set too soon in review, I have small fixes to add.

mogtofu33’s picture

Assigned: mogtofu33 » pdureau
Issue summary: View changes
Status: Needs work » Needs review
mogtofu33’s picture

Issue summary: View changes
StatusFileSize
new367.78 KB
mogtofu33’s picture

Issue summary: View changes
pdureau’s picture

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

Feature review

Page descriptions move to hook_help(), so they render in Drupal's help region:

Ok, and it looks better 👍

Last log is removed.

OK 👍

Checked:

  • /admin/structure/page-layout : looks great ✅
  • /admin/structure/display-builder/instances : looks great ✅
  • /admin/structure/views/display-builder : looks great ✅

So, the features changes are OK.

Code review

5 public static methods not clearly related and not covered by any interfaces have been added to DisplayBuilderHelpers class.

This may seem handy at first but this will add more work to do later: #3606952: Remove DisplayBuilderHelpers

So can we try to avoid this?

  • ::instanceCells(): why factoring such a simple data structure (2 properties, used only 3 times)?
  • ::statusBadge(): why not a render element or a SDC component?
  • ::publicationBadge(): used only by ::instanceCells(), can be private or moved there
  • ::hasUnpublishedChanges(): belong to Instance entity?
  • ::contentHash(): belong to Instance entity?

Documentation update

About the new docs/build-a-homepage.md, do we want to show how to do a "landing page" or the "homepage"?

Following our discussion of last weekly, where we talked about the Canvas' Page Content Entity Type, I guess it is the former.

By focusing on the landing page use case, we can get a smaller and friendlier documentation, with:

  • Step 1: Enable the modules
  • Step 2: Add the content type and activate display builder with overrides
  • Step 3: Create the landing page content and set a path
  • Step 4: Build the page display
  • Step 5: Check the page

Remaining tasks

Instance::isPublishedPresent() compares raw hashes, node IDs included, while the lists compare content without node IDs, so the builder and the lists can disagree

It may explain some weird published status I got last weekend 🤔

mogtofu33’s picture

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

Thanks for the review.

DisplayBuilderHelpers: the 5 new methods are gone, the class is back to its 1.0.x state, in line with #3606952: Remove DisplayBuilderHelpers.

  • ::contentHash() is now Instance::getContentHash(), next to ::getUniqId() on InstanceInterface.
  • ::hasUnpublishedChanges() moved to Instance. It stays static: the Instances panel compares hashes it batch-loads, without loading each buildable.
  • ::publicationBadge() is replaced by Instance::getPublicationStatus(), returning a PublicationStatus enum (Empty, Not published, Unpublished changes, Published).
  • ::statusBadge() is now a display_builder:status_badge SDC component.
  • ::instanceCells() is inlined in its 3 callers.

Documentation: I kept the homepage angle but told it through a landing page, so a search on either finds it. The tutorial is now "Build a homepage from a landing page": it creates a Landing page content type, and steps 1 to 4 alone build any landing page. The front page and page layout steps follow for those who want a homepage.

isPublishedPresent(): left for a follow-up, it compares hashes that include node IDs, so it can report unpublished changes where there are none.

pdureau’s picture

Status: Needs review » Needs work

I feel we may have reached some limits of "AI-acceleration" here 🤗

DisplayBuilderHelpers:

Moving the methods to Instance class are not only a cut-paste operation, we can add our little human touch here.

::hasUnpublishedChanges() moved to Instance. It stays static: the Instances panel compares hashes it batch-loads, without loading each buildable.

Why does it need to stay static now we are in the Instance entity and we always have an Instance entity instantiated each time we use it? The data is already here, let's use it.

I may be wrong, so I will try something in a separated branch. So, kept assigned to me.

::contentHash() is now Instance::getContentHash(), next to ::getUniqId() on InstanceInterface.

I will need to have a deeper look.

Documentation

Better but... something is still off. Maybe because it is written by an LLM. But let's move on and address this in a ticket about documentation in general.

mogtofu33’s picture

Fair point, the static methods on InstanceInterface are a leftover of the helpers' shape, not a need.

One constraint to keep in mind for your branch: the Instances panel already loads the instances with ::loadMultiple(), but it takes the published hash from DisplayReference, collected by each buildable while listing. That avoids loading every buildable, and its config or entity, a second time per row. So an instance method would still need a way to receive that hash from outside, or the panel pays one buildable load per row.

Created a wider approach and ctritics for our public contracts to ease contrib #3624396: Plan: Harden InstanceInterface, IslandInterface and DisplayBuildableInterface as public API before RC1, so some work here will be impacted and even could become not relevant, like deprecation of getUniqId()

And we have hash changes from discovery in #3555110: Symmetric translation, with specific bugs emerging, IMHO would be more efficient to handle that as child of #3624396: Plan: Harden InstanceInterface, IslandInterface and DisplayBuildableInterface as public API before RC1 instead of here.

pdureau’s picture

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

Carefull with the Hash, we have changes from discovery in #3555110: Symmetric translation, we specific bugs found, IMHO would be more efficient to try to tackle that as a follow up after merge of translation instead of here.

Thanks for the alert. So, i will not play risky games with ::contentHash() and stop here.

So, only one additional commit in the 415 MR:

commit a1174de87e84ed4ce9c71a5bdfc6c47d433fb605 
    Non static hasUnpublishedChanges()

Ready for review.

mogtofu33’s picture

Oups, edited while reply, so my edit highlight on my previous comment was to create child of #3624396: Plan: Harden InstanceInterface, IslandInterface and DisplayBuildableInterface as public API before RC1 after this and translation merge, in relation with #3606952: Remove DisplayBuilderHelpers and #3621008: Move DisplayBuildable form logic to a dedicated class to have more dedicated tasks if ok.

mogtofu33’s picture

So remaining contract points can go child of #3624396: Plan: Harden InstanceInterface, IslandInterface and DisplayBuildableInterface as public API before RC1: ::getContentHash() off the interface, ::isPublishedPresent() and ::hasUnpublishedChanges() giving one answer, which hash to use when, and whether PublicationStatus is public API. Nothing left to block this MR on that side.

mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Needs review » 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.

  • mogtofu33 committed 0fef5f7b on 1.0.x authored by pdureau
    task: #3623326 Consistent display lists: operations, publication state,...