Issue #3622574: Hand a workflow node out as an object rather than an array, so its shape has an owner

getNode() was getNodes()[$node_id] ?? NULL, so what a caller received was a plain array whose shape lived nowhere it could reach: written out by hand as an array{...} PHPDoc in three places that nothing kept in agreement, and read by indexing at sixty call sites. Forty-six signatures took array $node, and three of those are the plugin interfaces a module outside this project implements — somebody writing a timeout action was handed an untyped array and had to read our documentation to find out what was in it.

The object

WorkflowNode is a read view over the node as stored. It answers for the node's own structure — getId(), getLabel(), getTaskType(), getConfig(), getJoin(), getSplit() — and reaches a node feature's settings by name through getFeature() / hasFeature() / getFeatureKeys(), which is the one thing that cannot be its own getter because the features are not the kernel's to enumerate. Named as WorkflowDefinitionInterface names everything it answers for, since that is what hands nodes out.

It is a view, not a parse. The stored array is kept whole and toArray() returns it unchanged, because a version's executable hash is taken over the definition exactly as written: rebuilding it from parsed parts would reorder keys, or turn an absent key into an empty one, and cut a new version of every workflow the first time one was read. WorkflowNodeTest pins that.

Views are built per call rather than cached, and this is the one place the change costs something rather than saving it. PHP arrays are copy-on-write, so the old getNodes()[$id] handed back the stored map and indexed it for free: every view is now an object that did not exist before. Measured, on this branch: a run start-to-park builds 8 views and signal-to-end builds 5, and that count follows advances rather than node count, since 1-node, 5-node and 20-node chains each cost exactly 8. Nothing walks the whole map on the advance path. WorkflowNode::fromArray() costs 78ns, so a park pays about 0.6us against a measured 3.2ms, and a 20-node workflow pays 1.6us on the surfaces that do read every node. Caching them on the config entity instead would put a derived object into what gets serialized and leave a stale view behind every save.

The three plugin interfaces take one now: TimeoutActionInterface::onTimeout(), DeadlineProviderInterface::getDeadline() and shouldRearm(), and NodeFeatureInterface::toConfig(). That is the half worth doing for its own sake — it replaces documentation of a shape with the shape itself, for the people least able to read our docblocks.

The other direction

The definition contract both holders share answers with views (getNodes(), getNode()) and with ids (getNodeIds(), for a caller that walks by name and reads nothing off the nodes — which would otherwise build a view of each to keep the keys and drop the views).

getRawNodes() is the seam back to storage, and it is on WorkflowInterface, the editable workflow, not on the shared contract: writing and encoding only ever happen from the editable side, so an immutable version snapshot does not offer the door at all. The trait requires it as protected, so a snapshot satisfies the readers without exposing it. Its callers are the ones that build a node by merging submitted values and hand the whole map back through set('nodes', ...), and WorkflowVersionManager, which encodes the stored map into a snapshot.

docs/extending.md gains "Reading a node", which states the rule: getNodes() to read, getNodeIds() to walk by name, getRawNodes() only where the result is written back or serialized, toArray() as the seam for a single node in hand.

What the verification cost, because it is the interesting part

phpstan did the enumeration and could not do the checking. Updating the declared node shape first turned 208 errors into a worklist, and iterating to [OK] No errors fixed the whole typed surface. The suite then found four runtime failures phpstan had passed clean, every one of them a node view written back into storage:

• WorkflowVersionManager built its snapshot from getNodes(), so every node in every version JSON-encoded to {} — the engine saw nodes with no type and no config, and nothing parked at all. The field is array<string, mixed>, so nothing typed could see it.
• OrchestraCmForm did the same on the authoring write.
• Four tests read the map, changed one node, and re-saved it, which put objects into the config entity's nodes property.
• $model->getNode($id) ?? [] yields WorkflowNode|array, which passed a WorkflowNode parameter check and then hit a TypeError.

That is the mistake this shape makes easy to write and hard to see, which is why the docs say the rule and the commits name it.

Not in scope

Typed accessors for the kernel's own five features — a timer ladder as TimerDefinition[] rather than a list of maps — is where the strings inside a feature would disappear too. That is a second pass and a much larger one; this stops at the node.

What auditing the change found

Each fixed with the test that pins it, and worth listing because most of them are invisible to the type system:

• The nodes a run outlived. getNode() answers NULL where the definition no longer has the node a token stands on, which an unpinned run whose workflow was edited under it reaches. Eight readers on that path dereferenced it: the payload target and its completion scope, the message variable, the timeout a leaving token had, the interaction plugin, the payment step's own interaction, the ECA resume, the operation signal, the views node-label column. In the array shape $node['config']['x'] ?? '' answered for the absence for free; a method call does not.
• Completing a task whose node was deleted was a TypeError. OutcomeSignaler::resolve() took a node and the inbox handed it whatever getTokenNode() answered. It now takes the absence exactly as it already took a task type that no longer resolves — default outcome, no task-type extras — and the run walks the flow out of the node, which is what the engine does for the same absence on the signal path. This one was reachable by a holder pressing the button on their own open task.
• A node named with digits crashed the definition walk. machineName() keeps [a-z0-9_], so 123 is a name an author can type; PHP keys the map with the int, and getOutgoingFlows(string) under declare(strict_types=1) is a TypeError. getNodeIds() answers with strings, which is what the four sites walking a definition by name were already assuming. (The workflow editor has the same crash in its render, rename and submit paths; that is a pre-existing bug of its own and is not fixed here.)
• createForToken() minted a task for a token whose node the definition no longer has. The old code resolved candidates against an empty node, which answers "nobody" and tripped the guard that refuses; the refactor passed NULL, which reads as open to everyone — the one outcome an audience exists to prevent. It is refused outright now, and the throw is on the @api interface. The park step never reaches it, because createForParkedToken() asks for the task type first and mints nothing; both doors are asserted.
• The [orchestra:node-label] token stopped replacing for a node labelled with nothing. An unreplaced token is left in the text verbatim, so a reader of a notification would have seen [orchestra:node-label] where they had seen nothing before. NULL now means only that there is no node to label.
• array_keys($definition->getNodes()) built a view of every node to keep the keys and throw the views away — five sites, now walking by name. (StatusHistory still builds views to filter on a page the requester waits for, and stays that way: the object is what makes the predicate readable, and its 78ns per node sits behind the entity query under it.)
• Readers asking a node for the same key two or three times in one block — the timeout's three plugins, the operation form's outcomes and their labels, the payload target's name and scope — now read it once, as their neighbours already did.
• The hook example in orchestra.api.php and docs/extending.md still indexed the node it is handed, so a module following the documented extension point would have fatalled. DocumentedSurfaceTest pins that no documented example indexes a node — this example was written one change ago and broken by the next.

Two behaviour changes are deliberate: a step whose label is empty now shows its id in an inbox row, in the pending-actions list and in the views node-label column, rather than showing nothing. The old fallback caught only an absent label key, which the schema makes required, so it was already dead code.

For a module implementing one of the plugin interfaces

TimeoutActionInterface::onTimeout(), DeadlineProviderInterface::getDeadline() and shouldRearm(), and NodeFeatureInterface::toConfig() take a WorkflowNode where they took array $node. An implementation keeps its body and changes its signature; a body that indexed the array reads the same keys through getConfig() / getFeature(). docs/extending.md has the mapping.

Checks

Queries are unchanged, and not only by inspection: the production diff touches no getQuery, condition, loadMultiple, loadByProperties, execute or storage handle at all, and the two existing budgets over classes this change touches both pass, StatusHistoryTest asserting that no query asks for a run's whole token history and CurrentStepResolverTest that a whole page of instances resolves through one parked-token query.

phpcs Drupal and DrupalPractice clean over the whole module, phpstan level 5 [OK] No errors, cspell clean against a CI-shaped config, scripts/check-translations.php exit 0. The impacted kernel classes and both unit classes green against this branch in a lean root; the suite runs here.