Problem/Motivation

Child 2 of #3624396: Plan: Harden InstanceInterface, IslandInterface and DisplayBuildableInterface as public API before RC1. Needs #3627027: InstanceInterface: list the nodes of a slot.

The merge request is stacked on #3627027: InstanceInterface: list the nodes of a slot: it carries that commit until its merge request is merged. Review the last commit only.

The tree methods of InstanceInterface report a failure in four different ways:

  • moveToRoot() and moveToSlot() return FALSE for a node or a parent that does not exist, and moveToSlot() for a move into the node itself or one of its descendants.
  • remove() and setThirdPartySettings() do nothing, silently, for a node that does not exist.
  • setSource() throws, with a message that says nothing. attachToRoot() and attachToSlot() throw \OutOfRangeException when full, and do not declare it.
  • getNode() returns an empty array for a missing node. getParentId() returns NULL for a root node and for a node that does not exist alike.

A caller cannot write one error path, and cannot tell "nothing to do" from "this went wrong".

Proposed resolution

Apply the rules of the plan to every tree method:

  • getNode() returns ?array: NULL for a missing node.
  • getParentId() throws InvalidNodeException for a node that does not exist. NULL keeps meaning "at the root".
  • moveToRoot() and moveToSlot() return static. They throw InvalidNodeException for an unknown node or parent, and for a move into the node itself or its descendant, checked before the cardinality. A full root or slot throws \OutOfRangeException, as today.
  • remove(), setSource() and setThirdPartySettings() return static and throw InvalidNodeException for an unknown node.
  • Every tree method declares what it throws with @throws, attachToRoot() and attachToSlot() included.

In the module:

display_builder_ai and the dev tools call these methods. Checked: every call already guards its read, no change needed there.

This breaks a return type and adds exceptions, which a deprecation cannot express. So it lands before RC1, in a beta, with a change record.

Remaining tasks

  • Decide whether an unknown slot ID throws. attachToSlot() and moveToSlot() store any slot key on an existing parent today. Throwing InvalidNodeException needs every source with slots to list all the slots it accepts.
  • Publish the change record when this is merged.

User interface changes

None intended. A refused change shows the error toast of #3626659: Paste, duplicate and preset insert end in a server error when the target is full or gone.

Moving a node that no longer exists, or into its own descendant, now shows that toast instead of "[attachToSlot] moveToSlot failed with invalid data".

API changes

  • getNode(): ?array, was array.
  • getParentId() throws for a missing node.
  • moveToRoot() and moveToSlot() return static, were bool, and throw where they returned FALSE.
  • remove(), setSource() and setThirdPartySettings() return static, were void, and throw for an unknown node.
  • \OutOfRangeException now carries a message naming the full root or slot.
  • New protected ApiControllerBase::getExistingNode(). SourceTree::isDescendant() is public.

Data model changes

None.

Issue fork display_builder-3627028

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’s picture

mogtofu33 commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented
Priority: Normal » Major

mogtofu33 opened merge request !451

mogtofu33’s picture

mogtofu33 commented
Assigned: Unassigned » pdureau
Issue summary: View changes
Status: Active » Needs review

What changed

  • The seven tree methods follow the rules of the summary. One private helper, Instance::getExistingNodeData(), throws the one "Node %s does not exist." message for the six methods that need it.
  • moveToSlot() refuses a move into the node itself or its descendant before it checks the cardinality, so a cyclic move into a full slot is InvalidNodeException, not \OutOfRangeException.
  • ApiController::attachToRoot() and attachToSlot() lose their "move failed" error branch: the exception subscriber of #3626659: Paste, duplicate and preset insert end in a server error when the target is full or gone answers instead. The complexity of ApiController goes from 57 to 55, for a PHPMD limit of 60.
  • update(), thirdPartySettingsUpdate() and saveAsPreset() read the node through ApiControllerBase::getExistingNode(). get() and the islands keep an empty array for a missing node. ViewPanelBase::replaceNode() renders nothing for one, where it raised an "Undefined array key" warning.
  • The two tests pinning the silent returns are turned around.

Tests

  • InstanceTest: every method on a missing node, both cyclic moves, a cyclic move into a full slot, the returned instance, and a refused change adding no history step.
  • ApiControllerTest and ApiActionsControllerTest: update, third party settings, both moves, delete, save as preset, paste and delete styles on a missing node.
  • Kernel and unit suites pass, and so do the move, contextual, history, nesting, dropzone, preset and style Playwright specs.

For review

  • Unknown slot IDs, from the remaining tasks: attachToSlot() and moveToSlot() still store any slot key on an existing parent. I left it out: throwing needs every source with slots checked first. Decide it here, or in a follow-up?
  • The change record is drafted, to publish once this is merged.
pdureau’s picture

pdureau commented

Waiting #3627027: InstanceInterface: list the nodes of a slot strategy to be acted.

mogtofu33’s picture

mogtofu33 commented

Rebased with #3627027: InstanceInterface: list the nodes of a slot, ready for first review.

Complementary review

Unknown slot IDs

attachToSlot() and moveToSlot() check that the parent exists and that the slot is not full. They never check that the slot exists. Two cases, checked with a kernel test on this branch:

  • A slot the parent does not declare, attachToSlot($component_id, 'no_such_slot', ...): accepted and saved under component.slots.no_such_slot. ui_patterns renders only the slots of the component definition. The node is stored but never shown, neither in the Canvas nor on the page.
  • A parent whose source has no slots, a text source for example: accepted, and a node ID is returned. The node is gone at the next load. SourceTree writes children back only into sources implementing SourceWithSlotsInterface.

The builder UI never hits this, because its drop targets come from the slot definitions. Code does. display_builder_ai takes the slot ID from the model's tool call, so a made-up slot name ends as a hidden node and a "component added" answer.

Proposal: both methods throw InvalidNodeException when the parent source does not implement SourceWithSlotsInterface, or when $slot_id is not a key of its getSlotDefinitions().

What it costs:

  • getSlotDefinitions() becomes binding: it must list every slot the source accepts. Its docblock says so, and the change record tells contrib sources. The three sources in Display Builder already comply: component slots from the SDC definition, layout regions, and page layout regions of the default theme.
  • A component whose definition is gone, after its theme or module is uninstalled, declares no slots. Adding into it is refused, and its slots cannot render without the definition anyway.
  • After a default theme switch, a page layout refuses the regions of the old theme. Those do not render either.
pdureau’s picture

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

Great.

  • mogtofu33 committed 4102437c on 1.0.x 
    task: #3627028 InstanceInterface: one failure style for the tree methods...
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.