Issue #3618840: Audit of duplicated logic: the exactly-once timeout claim written twice, a migration preview that cannot follow the migration, three definitions of the definition an instance runs under, and a mail attachment mapped twice

Fixes every finding of the audit on [#3618840], one commit per finding.

Nothing here changes what the module does, with three deliberate exceptions, all named below. The whole point is that a computation written more than once has no owner, so each commit gives one of them an owner and deletes the copies.

The claims and the queries

  • TimeoutSweeper::claimDueDeadline() — the exactly-once claim was written twice, in fireInner() and fireTimerInner(), about 25 lines each. Both had the same two ways of firing twice, so a change to one rule left timers and timeouts on different ones with nothing to fail. Each call site keeps its own comment on why its snapshot can be stale, which is where the two genuinely differ.
  • WorkflowVersionManager::surveyMigration() — the Drush command and the operator form each carried a 22-line survey(), and both re-derived the instance selection migrate() performs for real. The preview and the migration now select through one private migratableInstanceIds(), and the survey is a named MigrationSurvey rather than a two-element tuple. Both callers lose the entity type manager they injected only to run it.
  • DefinitionResolver::nodeFor() — "the node a token stands on" was written out at sixteen sites, seven of them the same four-line ternary. Fourteen let definitionFor()'s InvalidArgumentException escape, including an access check and three forms, although effectiveDefinition() was written for that case and none of them called it. definitionFor() and effectiveDefinition() were also the same body but for the throw; both now read the pinned version and the live config through one private step each.
  • InteractionResolver held a third, byte-identical copy of that resolution and injected the version manager to run it. It takes the engine's resolver instead. Its four public methods that accept an instance keep reading through it, because a caller may hold an instance that is not the token's.
  • InteractionResolver::parkedQuery() / parkedTokens() — four methods each built the same three conditions with the same id ordering and walked the ids dropping timers. Same queries, same order, same results.
  • PinnedPayment::queryForInstance() — the class docblock says it is the one place the token-pin query lives, and two cleanups went around it to rebuild it. Each now narrows the shared query instead. Its own "load the first id or nothing" tail, written twice in one class, becomes firstOf().
  • ChunkedDeleteTrait::deleteByInstance() — four cleanup hooks paired the same one-condition query with the chunked delete by hand.

The plugins and the handlers

  • RoleAudienceBase — the roles and roles_variable audiences differed only in where the role names come from, and repeated the capped member query, the cap constant, the oversized-pool warning, candidates() and viewerTokens(). The two log messages become one that names the plugin, which is more than either said. roles gives up its constructor entirely.
  • AudienceVariableTrait — the email, users and roles variable audiences each opened by trimming the configured name, reading the variables, testing for the key and coercing a scalar to a list, and each carried the same single-setting configuration form. Only the field description stays per-plugin, since it is the part that says what the values are for.
  • CurrentValueFilterBase / CurrentUserFilterBase — the completer and on-behalf-of filters had identical query() bodies, and five filters repeated the same constructor, factory and cache-context scaffolding. The three user filters are now a docblock and an attribute each.
  • AssignmentCacheTrait — four Views field handlers in two modules carried identical getCacheTags() and getCacheMaxAge() bodies under freshly written comments. The contexts stay with each handler, since one carrying a return target varies with the URI and one naming a person does not.
  • NotificationAttachment::toMailArray() — both mail channels carried a byte-identical private mapper, docblock included, each claiming the channel owns the mapping. Two identical channels no longer make that true.
  • CommentConfigTrait — the comment task and comment operation built the same two settings twice, differing only in the word for the person acting.
  • SubprocessTask now takes the shared completion-scope element.

The three behaviour changes

  1. A non-SQL query backend is refused by all five "current something" filters. Three threw already; the initiator and tenant filters silently dropped their restriction, which on a personal list means showing everybody's rows. The refusal now names the plugin.
  2. The subprocess step's completion-scope select gains its description. It built its own copy and dropped it, so the setting was explained on six node types and unexplained on the seventh. The string is already translated.
  3. InstanceWorkItemCleanup is gone, its delete folded into InstanceCleanup. Its docblock said user tasks were owned by a submodule and that the engine knew nothing about them, from a namespace inside the engine, beside the work item entity. The tasks are now deleted before the tokens they name, as the subprocess children already were.

Translations, docs and tests

Interaction settings and the interaction-plugin description are declared in orchestra_interaction but appeared only in orchestra_interaction_task's catalogue, so a site running the interaction module without the task module showed both in English; they move to the declaring module, and two strings the base catalogue already carries stop being duplicated. ReviewInteractionBase stops opening on what happened once handler_url was retired.

Three test gaps, all on code these commits touch:

  • the migration survey, asserted against the migration itself rather than against a second expression of the same rule; seen to fail with the target-version condition dropped (2 counted, 1 moved);
  • the roles_variable audience, which had no test of its own at all; seen to fail with the plugin's role source stubbed out;
  • the negated half of a personal Views lens, where the three filters used to disagree, and which no shipped display exercises.

Verification

PHPStan level 3 clean over the branch (729 files analysed, checked with --debug rather than trusting [OK]), PHPCS clean over the module root with Drupal,DrupalPractice apart from one pre-existing DrupalPractice warning on orchestra_views.routing.yml that predates this branch, and the affected kernel classes run green locally against a mirror root, since a worktree is invisible to extension discovery. The full suite is this pipeline's job.

Merge request reports

Loading
Loading