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

What the full file-by-file audit of 1.x at 500e8cbc found that is not a behaviour change: one dead value with a false docblock, three duplications, and the prose and standards findings. Grouped as one sweep, as #3620621: Prose, dead code and naming sweep from the pre-beta audit was.

A dead value, and a docblock that is false

DefinitionResolver::getStartRefusal() returns a reason string, and its only caller, WorkflowEngine::canStart(), discards it for a boolean, so nothing reads the reason. Meanwhile WorkflowExecutor::start() throws the same four refusals as its own sprintf strings, and that message is the one actually surfaced, logged by the webform handler when a submitted start is refused. getStartRefusal() claims in its docblock to answer in the wording the engine own refusal carries, and two of the four do not match: Unknown workflow against does not exist, and is disabled so it cannot be started against is disabled. Making getStartRefusal() the single source and having start() throw with it makes the two agree by construction instead of by hand.

The bounded queue drainer, reimplemented unbounded

AdvanceQueueDrainer is the shared, bounded advance-queue drain, marked api and injected in five places, and its fifty item cap exists because the queue is shared across every instance. OrchestraUiController::run() hand-rolls the loop instead, with no bound, so an administrator clicking Run pending steps on a site with a backlog drains every tenant work in one web request until it times out. It also has only a catch on Throwable, so a token that asked for its retry backoff is released, logged as an error, reported as a pending step failed, and the whole drain aborts, which means one backing-off token stops every other instance advancing. RequeueException and SuspendQueueException get the same treatment, where the drainer releases and stops quietly.

The button also cannot report what it did, because drain() returns nothing: its one message asserts that the queue was emptied, which a bounded drain cannot know, and offers cron as the only way to take the rest, where clicking the button again would take the next batch. drain() now answers with a DrainOutcome - emptied, capped or deferred - and the button says which, because the next thing to do differs in each case. None of the three answers claims more than the drain checked: 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 retry backoff is still queued, so the emptied answer is about what can run now and not about the queue being empty.

Three copies of one lookup, and four bypassed accessors

getCurrentVersionId(), resolveVersion() and refreshCapturedTranslations() each repeat load the one snapshot entity for this workflow, or NULL. And four call sites reach through a field item where the interface declares a typed reader: CurrentStepResolver and RetentionManager read the instance where TokenInterface::getInstanceId() exists, and WorkItemManager reads the tenant and the label through field items at two sites while using the typed reader elsewhere in the same class.

A banned word, sixty-four times

memo, memoize and memoized appear in comments and docblocks across seventeen files, most of them in AssignmentMatcher and WorkflowVersionManager. Core says static cache, seventy-six times, and never memo.

Standards and accessibility

RetentionPreviewTrait uses the count placeholder in a t() call, as do five dt() sites in the Drush commands; the count 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, and the argument is what reaches a translator, which does not improve for being a habit core also has. InstanceListForm tableselect is the module only one and the only table the project renders without a caption, and the caption is asserted in the rendered markup as well as in the render array, because a source sweep cannot see one dropped on the way to the page and a tableselect is the element where that could happen: it themes through a suggestion of its own and rebuilds the element to add the checkbox column, so a screen reader gets an unnamed table of checkboxes on the main administrative listing. Its class docblock still documents only delete selected, where it now also offers cancel selected and migrate selected to current, each running a batch. And composer.json suggest lists potx, described in its own text as development only with nothing at runtime using it, where it is correctly in require-dev already; the kessai entry beside it cited a kessai issue as the current example of that API still settling, and that issue is now closed.

The largest duplication in the tree, in the tests

An eight-line-window duplication sweep over all 848 PHP files finds no removable duplication in production code, because the recurring blocks there are mandatory framework boilerplate: create() factories in eighteen plugins and DefaultPluginManager constructors in twelve managers. All of it is in the tests, where there are 361 Workflow::create() fixtures across 178 files, and the commonest by far is one shape: start, a step, end, wired by two flows. The tests directory already holds twelve shared traits, and ParkedTokenTrait docblock is the case for this one: the question almost every engine test asks was written out in each of them, and six implementations had grown apart. The reading half was extracted and the building half was not.

The trait builds that shape and takes the differences, and 151 are converted, across every module and not only the base one. The text pattern that drove the first pass is not the criterion, so the tree was re-read by shape afterwards - bracket-matching every create and counting its nodes and flows - which found six more still written out plus one the rebase brought in: a comment inside the nodes array, a merge into the middle node, a variable node id or an extra top-level key was enough to hide a fixture from the pattern. It builds the shape and never the meaning: the middle node type, its config and its assignments stay with the caller, because those are what a test varies. The 217 fixtures that are some other shape - a fork, a join, several branches, flows written as a keyed map - stay written out, and should, because a helper with a parameter per variation would read worse than the array it replaced.

Each conversion was checked to build the same array as the literal it replaced, element by element, rather than trusted to a pattern. That check is what refused two of them: a fixture whose flows are a keyed map is a different array from the list the trait builds, and one carrying a comment explaining a key would have had the prose stranded against a key no longer written there.

Tests

The behaviour findings get behavioural tests: the start-refusal wordings asserted equal to the exception messages for all four conditions, the drainer path asserted to delay a backoff item rather than abort, and each of the three outcomes asserted to reach the operator as its own message, including that a capped drain offers the next batch rather than claiming the queue is empty. The prose and standards findings get standing structural checks rather than one-off assertions, because a sweep that is not a standing check reopens one hop away: a check that the tree carries no banned vocabulary, a check that no t() call uses the count placeholder, and a check that every table the project renders carries a caption. Each was proved by planting the defect it looks for and reading back the line it named. 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, so the table was skipped and the check passed; the placeholder check went the other way and read a comment explaining why a sentence does not use the placeholder as a use of it. Comments are blanked first now, keeping every offset and line, and a table whose array cannot be walked to its end is reported as unreadable rather than skipped, because something will defeat a delimiter counter eventually and the table has to fail loudly when it does. The vocabulary list is closed against the spell checker rather than by taste: every form the checker accepts is banned here, and the three it rejects are described rather than spelled, since a word it rejects cannot be written in a file it reads. The accessor and lookup extractions are covered by the existing versioning and current-step tests.

AI-Generated: Yes (Claude Code performed the audit that found these, drafted this summary, and wrote the fixes and their tests on the merge request. I reviewed the findings against the source, and ran the tests, before posting.)