Duplication, dead values, vocabulary and accessibility sweep from the full audit

Nine findings from the full audit that share one shape: something is said twice and the copies have drifted, or something is written down that nothing reads. Each is its own commit.

Two readings of the same refusal

DefinitionResolver::getStartRefusal() exists so a doorway can ask, before it offers a form, why a run would not start. WorkflowExecutor::start() refuses for the same four reasons, and its message is what an operator later reads in the log. Both spelled the reasons out separately, and two of the four had drifted: Workflow "x" does not exist against Unknown workflow "x", and is disabled against is disabled, so it cannot be started — while the resolver's own docblock claimed it answered in the wording the engine carries.

The four are now REFUSAL_* templates both sides format, and a top-level start() asks getStartRefusal() and throws what it said. Only at the top level: a subprocess runs in its parent's tenant, which is not the tenant that method judges.

StartRefusalTest asserted only that both sides refuse, which is exactly what let the wordings drift. It now also asserts the thrown message is the reason the doorway was given — an assertion that fails on 1.x.

A drain loop written twice, and only one of them bounded

"Run pending steps" had its own copy of AdvanceQueueDrainer's loop. The copy differed in three ways, all of them invisible until a site has a backlog:

• no bound, so it drained every tenant's queued work in one web request until the request timed out;
• it caught only Throwable, so a token asking for its retry backoff was released, logged as an error and reported as a failure, and the whole drain stopped — one backing-off token halting every other instance;
• a requeue and a suspended queue were treated as failures.

The count goes with the loop: a bounded drain's count says how much of the queue it was allowed to take, not how much work there was, and an operator reading "50" cannot tell the two apart.

What an operator does want to know is whether anything is left, and drain() returned void, so the button's message could not say. It answers with a DrainOutcome now — Emptied, Capped or Deferred — and the three cases read differently, because the next thing to do differs: nothing is left; a batch was taken and pressing the button again takes the next one; or a step asked to be retried later and cron takes it. The old single message asserted a finish it could not verify, and pointed only at cron even where pressing the button again would have done.

None of the three answers claims more than the drain checked, which took two corrections of its own: reaching the bound is not proof that work is left, so a queue holding exactly the bound reports itself emptied rather than offering a next batch that is not there; and an item delayed by a backoff is still queued, so the emptied answer is about what can run now and not about the queue being empty.

RunPendingStepsTest builds a backlog past the bound, which is the only thing that tells a bounded drain from an unbounded one, and asserts the action takes the bound exactly, leaves the rest queued, and says so — that it offers the next batch rather than claiming the queue is empty. Both corrections are pinned too, each by an assertion that fails without it.

One snapshot, asked for three ways

Three methods in WorkflowVersionManager each re-derived that a workflow has at most one version snapshot and that loadByProperties() answers with an array whose first element is it — each with its own reset() and its own instanceof guard, repeated further down to use what it had found. loadSnapshot() answers "the snapshot, or nothing" once, typed.

The commonest test fixture, written out 151 times

A census found over 350 Workflow::create() fixtures in the suite, and the commonest by far was one shape: start, a step, end, wired by two flows called f1 and f2. Every one spelled out the same three nodes and the same two flows to reach the one thing its test was about, usually a single key in the middle node's config.

WorkflowFixtureTrait builds the shape and takes the differences. It builds the shape and never the meaning — the node's type, config and assignments stay the caller's, because those are what a test varies. A fixture that needs a fork, a join or several branches still writes itself out, and should.

Every replacement was checked to build the same array as the literal it replaced, element by element, rather than trusted to a pattern - the workflow's own key order aside, which the hashed definition does not cover. A handful of fixtures share the outline and are still written out, each for something the trait cannot take: flows named something other than f1 and f2, a flow carrying a condition, an end node whose type or label the test varies, and one that is decorated with third-party settings before it is saved, where the trait saves as it builds.

The pattern that drove the first pass is not the criterion, which is why seven of the 151 are hand conversions found by re-reading afterwards: a comment inside the nodes array, a + merge, a variable node id or an extra top-level key was enough to hide a fixture from it, and the rebase onto the merged !462 (merged) brought in one more that the pass never saw.

The commonest of those has a wait in the middle, so createWaitWorkflow() fills that node's three keys in as well and takes whatever the test adds to it. It merges with array_replace() rather than +, because a node's executable hash is taken over the definition as written and reordering its keys would change every affected version's hash for nothing.

SynchronousExecutionTest had a private createLinearWorkflow() of its own that built a different shape — start straight to end — so it is renamed createEmptyWorkflow() for what it builds.

Four fields read past their own accessors

$token->get('instance')->target_id where getInstanceId(): ?int exists, $task->get('tenant')->value for getTenantId(), $task->get('label')->value for label(). The interfaces declare all three, so the long form is a second way of asking that the type checker cannot see and a rename would not follow.

A word Drupal does not use

Core says "static cache" 76 times and "memo" never. This project had reached 64 occurrences of memo, memoize and memoized across 17 files, most of them in the two classes a reader of the engine meets first.

A number that is not a plural

Six strings passed a count as @count to t() or dt(). That placeholder is what a plural translation is keyed on, so the string reaches a translator as one msgid with a placeholder they cannot pluralize, and the catalog freezes whichever grammatical number the English happened to use. Core is not the argument here, and was checked rather than assumed: it does this itself in a handful of places. The argument is what reaches a translator, which does not improve for being a habit core also has. These sentences are only reached where the number is many, beside a formatPlural() that handles the rest, so the placeholder is named for what it holds.

A table nobody can name

The main administrative listing of process instances was the only table this project rendered without a caption, every other one already having had it. A tableselect renders through core's table.html.twig, so a screen reader met a grid of unnamed checkboxes; a sighted reader takes the name from the heading above it, and nobody navigating by table gets it at all. No linter reports this — a table without a caption is valid HTML and valid Drupal.

The key is asserted over every rendered table by a standing check, and separately in the markup: InstanceListDisplayTest renders the list and asserts the <caption> and its text, because a source sweep cannot see a caption dropped on the way to the page, and a tableselect is exactly the element where that could happen — it themes through a suggestion of its own and rebuilds the element to add the checkbox column.

Two suggestions that are not suggestions

drupal/potx is not an optional feature of Orchestra; it is the extractor scripts/check-translations.php runs, and require-dev is where it already is. The drupal/kessai entry also named another project as a precedent for how it pins, which says nothing to anyone reading Orchestra's dependencies, and cited a kessai issue as "the current example" of that API still settling — an issue that is now closed, so the sentence pointed at a finished rename and dated the file. The reason it tracks 1.x-dev stands without either.

Tests

Three findings are prose or configuration, which is exactly why a sweep alone would not hold them — they come back one comment, one table at a time. So they are standing checks over the project's own files rather than one-off corrections:

• HouseVocabularyTest — no file says "memo", and @count appears only in a plural call;
• TableCaptionTest — every rendered table says what it is a table of.

Each was proven by planting the defect it looks for and watching it name the exact line. SourceScanTrait holds the walk both need: finding the construct a point in the source sits inside, over brackets for one and parentheses for the other.

Both walks read code as code, which they did not at first: a comment is prose, so an apostrophe in one is not a string and a bracket in one closes nothing. A table whose array held one apostrophe in a comment walked to no closing bracket, and finding no bracket read exactly like finding no need for a caption — the table was skipped and the check passed. The placeholder check went the other way: a comment explaining why a sentence does not use @count was read as using it. Comments are blanked first now, keeping every offset and line, and both cases are pinned on source rather than on whatever the shipped files happen to contain today.

And the general form of that failure is closed, not just the comment case: a table whose array cannot be walked to its end is reported as unreadable rather than skipped. Something will defeat a delimiter counter eventually, and when it does the table has to fail loudly instead of passing as one that needs nothing.

StartRefusalTest and RunPendingStepsTest cover the two behavioural findings, both failing on 1.x.