Problem/Motivation
The four View panels form this chain today:
IslandPluginBase └── BuilderPanel (id: builder, "Canvas") 743 lines ├── TreePanel (id: tree, "Navigator") 171 lines └── WireframePanelBase (abstract) 246 lines └── ScaffoldPanel (id: scaffold, "Scaffold") 116 lines
BuilderPanel is the Canvas and the base class of every other View panel, so it holds two unrelated jobs:
| Concern | Methods | Used by |
|---|---|---|
| View panel base | buildRootDropzone(), digFromSlot(), buildComponentSlot(), buildNodeAttributes(), buildSlotAttributes(), resolveComponentInfo(), replaceNode(), the island event handlers |
all four panels |
| Canvas real render | buildComponentRealRender(), renderSource(), useAttributesVariable(), hasMultipleRoot(), isEmpty(), buildSingleBlock() and its placeholder policy |
Canvas, plus one call from ScaffoldPanel |
TreePanel and WireframePanelBase override both buildSingleComponent() and buildSingleBlock(), then inherit roughly 350 lines of Canvas rendering they never call. Consequences we already hit:
- Every improvement to the Canvas's placeholder and message policy lands, dead, in three panels that cannot reach it. The block placeholder work did exactly this.
BuilderPanelis the largest island by 190 lines and keeps growing, because it is where anything shared has to go.- Two services injected for one branch of one Canvas method are constructed for every View panel on every builder page.
Nothing here is user-visible. It is a maintenance problem: the class that new contributors read first is the one where the least of it applies to them.
Proposed resolution
- Add
ViewPanelBase(abstract,src/Plugin/display_builder/Island/) holding the shared half of the table above, moved verbatim, names unchanged. - Move the Canvas real render into a trait, e.g.
RealRenderTrait:buildComponentRealRender(),renderSource(),useAttributesVariable(),hasMultipleRoot(), the render-emptiness check. BuilderPanel extends ViewPanelBase, uses the trait, and keepsbuildSingleBlock()and the placeholder policy, which are its own.TreePanel extends ViewPanelBaseandWireframePanelBase extends ViewPanelBase.ScaffoldPaneluses the trait as well — see the trap below.
The trap. ScaffoldPanel::buildSingleComponent() calls $this->buildComponentRealRender() for its allowlisted layout components, and parent::buildSingleComponent() for everything else. Moving WireframePanelBase off BuilderPanel removes the first of those unless the real render is reachable from Scaffold. That is the reason for the trait rather than a plain base class split.
Cleanup found on the way, worth folding in: BuilderPanel::isEmpty() duplicates RenderableBuilderTrait::isRenderEmptyOrFailing(), which the class already has through IslandPluginBase and which PreviewPanel calls. Drop the private copy instead of moving it.
Remaining tasks
- Add
ViewPanelBaseandRealRenderTrait, move the methods, no behaviour change in the move commit. - Re-parent
TreePanelandWireframePanelBase; wireScaffoldPanelto the trait. - Delete
BuilderPanel::isEmpty()in favour of the trait method. - Kernel:
BuilderPanelTest,TreePanelTest,ScaffoldPanelTest,DropzoneTestmust pass unchanged. If one needs editing, the move was not behaviour-neutral. - Playwright:
tree.spec.ts, plus a pass over the four View panels in a browser: drag between panels, contextual menu, delete, undo. phpcbf,php-cs-fixer, PHPStan on the module, cspell.
User interface changes
None. No plugin id, island attach_to, profile config, keyboard shortcut or stored data changes.
API changes
Internal to the module, but real for anyone subclassing a View panel: the parent of TreePanel and WireframePanelBase changes, so a custom panel extending either and relying on Canvas-only methods would need the trait. Method names and signatures are otherwise unchanged. The module is beta; noting it in the release notes is enough.
Data model changes
None.
Issue fork display_builder-3615437
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
Comment #3
mogtofu33 commentedComment #4
mogtofu33 commentedComment #5
pdureau commentedCool, it will make the work on
BuilderPanelsafer.So, we now have:
I move to RTBC but, before merging, can we check?
WireframePanelBase, only extended byScaffoldPanel, in this new classes hierarchy?RealRenderTrait?Comment #6
mogtofu33 commentedWireframePanelBasewas a base because of legacy layers, so yes now it make less sense and is more something for a future wireframe island different than scaffold. But surely as it is a guess, better to merge scaffold, and add a base when really needed.I guess
slotHandlerscould fit inRealRenderTraitif the scope is the same and it do nothing to the tree but is it? If it act on allViewPanelBaseprobably better to have it's own trait if we have repeating methods? Even more if it is transverse to more islands.Comment #7
pdureau commentedThe scope is the same. So,
RealRenderTraitseems the right place for #3594176: Slot handlers as island plugins because it acts only inBuilderPanelandScaffoldPanelComment #9
mogtofu33 commented