Problem/Motivation

The Instances panel and the admin instances list both loop over every DisplayBuildable plugin, so a buildable from contrib or a submodule is discovered. Neither lists it by default. The panel calls collectDisplays(), which returns an empty array in DisplayBuildablePluginBase: the opt-in is deliberate, but no page in docs/ mentions it. The admin list calls collectInstances(), which has no default in the base class, so every buildable must implement it. The list shows only what each plugin returns, and the entity_view and page_layout implementations call initInstanceIfMissing(), so loading the list saves Instance entities.

Proposed resolution

Load the admin list from Instance storage, as an entity list builder normally does, and read each row's plugin from its buildable field, as the Context column already does. Every instance is listed, whatever plugin made it, and nothing is written. Alternatively, keep the list as is and give collectInstances() a read-only default in the base class, loading the instances whose ID starts with the plugin's instance prefix. Either way, document what the panel and the list call on a buildable.

Remaining tasks

  • Decide: storage query, or a base class default.
  • Check whether instances can outlive their display, and how the list shows them.
  • Kernel test listing the display_builder_test buildable.
  • Docs.

User interface changes

None for the existing buildables. Contrib buildables appear in the admin list.

API changes

None with the storage query. With the alternative, DisplayBuildablePluginBase gains a default collectInstances(), which is BC-safe.

Data model changes

None.

AI-Generated: Yes

Issue fork display_builder-3629063

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

mogtofu33 created an issue. See original summary.

mogtofu33 opened merge request !467

mogtofu33’s picture

mogtofu33 commented
Status: Active » Needs work
pdureau’s picture

pdureau commented
Issue tags: -display_builder-1.0.0-beta9 +display_builder-1.0.0-beta

beta9 has been released. Changing the tag.

pdureau’s picture

pdureau commented
Assigned: mogtofu33 » pdureau

A big rebase is needed. I can help.

The big deal is the removal of DisplayBuildableInterface::collectInstances(), with removal from all buildable plugins: PageLayout, EntityView, EntityViewOverride, ViewDisplay, PatternPreset, TestDisplayBuildablePlugin, TestConfigBuildablePlugin...

However, more DisplayBuildableInterface::collectInstances() callers has been added in 1.0.x: PatternPresetListBuilder, PatternPresetBuildableTest

Also, InstanceListBuilderTest has already been added by #3631292: Filter, sort and page the presets list.

So, it may a big change so I may create a MR dedicated to this rebase.

pdureau opened merge request !526

pdureau changed the visibility of the branch 3629063-instances-list to hidden.

pdureau’s picture

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

Rebase

Rebased in a dedicated branch: https://git.drupalcode.org/project/display_builder/-/merge_requests/526/...

As expected, it was pretty complex.

Here are the differences between MR 467 & MR 526.

  • 20 files carry exactly the same changes.
  • 2 have different changes:
    • modules/display_builder_ui/src/InstanceListBuilder.php: storage query inside upstream's URL-based load().
    • modules/display_builder_ui/tests/src/Kernel/InstanceListBuilderTest.php: upstream's tests, plus your two, run as an administrator.
  • 2 are changed only in MR 526:
    • modules/display_builder_ui/src/PatternPresetListBuilder.php
    • modules/display_builder_views/tests/src/Kernel/InstanceViewsAccessControlTest.php

Next steps?

I am not 100% sure of my understanding, and the MR was in "Need work" for 20 days, so I guess I may be missing something, but it looks ready for review to me.

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

✅ Tested and works — on 1.0.x, deleting the entity_view__node__page__default instance and reloading /admin/structure/display-builder/instances saved it again with a fresh "Updated" time, and the two entity_override__node__* instances were missing. With MR !526 applied (plus drush updb and drush cr), reloading the list no longer recreated the deleted instance, and both Entity view override rows now show. Listing is read-only and lists instances from storage.

Code review
  • MR !526 at 8f2b122, pipeline green; it is rebased on current 1.0.x.
  • testListsInstancesOfAnyBuildable would fail without the fix, because the test buildable has no collectInstances().
  • Non-blocking: the MR removes DisplayBuildableInterface::collectInstances(), but the IS says "API changes: None" for the storage query option, so please update the IS.
pdureau’s picture

pdureau commented
Assigned: Unassigned » mogtofu33

Non-blocking: the MR removes DisplayBuildableInterface::collectInstances(), but the IS says "API changes: None" for the storage query option, so please update the IS.

Indeed, I removed DisplayBuildableInterface::collectInstances(), believing it was the expectation because it was already done in Jean's MR 467, but the issue description doesn't ask for that. I guess it is OK?

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented

Yes, I think removing it is fine. After the MR, nothing in the module calls collectInstances() any more, so keeping it would leave dead API. A contrib buildable that still implements it does not break; only an external caller would. That fits the API hardening before RC1 in #3624396.

Please just update the IS: set the chosen resolution (storage query), and under "API changes" note that DisplayBuildableInterface::collectInstances() is removed.

  • mogtofu33 committed def1b3da on 1.0.x authored by pdureau 
    task: #3629063 Instances list: load instances from storage, not from...
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.

mogtofu33’s picture

mogtofu33 commented

#3629063-11: Instances list: load instances from storage, not from each buildable Yes it's ok, I have a beta10 change record in draft (locally) for that.