Issue #3622387: Name the control that adds a row, and pin the number to the list

Frank Mablyrequested to merge
issue/orchestra-3622387:3622387-name-the-row-to-add into 1.x

A follow-up to !470 (merged), from auditing the merged branch. Three findings, all in the family that issue was about, none of them a behaviour the editors get wrong on the page today.

The add control wore a row's name

Every sequence editor ends with an add control: a plugin select plus an "Add timer" button, in a plain container rather than a group. The select carried $labels['field'] — the same title every row's own select carries. Naming the rows in !470 (merged) made it the odd one out rather than fixing it:

group "Timer 1"  -> Action, combo box
group "Timer 2"  -> Action, combo box
(no group)       -> Action, combo box     <- the add control

The third is not a row. A reader tabbing through a two-timer ladder heard "Action" three times with nothing saying which one adds a rung, and the control is the one thing here that sits outside every fieldset, so its label is all they get.

It now names the row it would create, through a new getAddRowTitle() beside getRowTitle(): Timer to add, Assignment to add, defaulting to Row to add. No new getLabels() key, so nothing extending this @api class breaks, and a third-party sequence feature is distinguishable from its own rows without changing anything.

$labels['field'] is deliberately not interpolated into a sentence: it is a sentence-cased label ("Action"), so "New @field" would render as "New Action" in English and would need a different structure in other languages anyway.

The number was pinned only where it could not go wrong

!470 (merged) argues, at some length, that a row's number is counted as the list stands rather than taken from its stored key, "because removing a row leaves a gap in those keys". The test seeded two contiguous rows, where the position and the key agree, so i0 -> 1, i1 -> 2. A regression to (int) substr($key, 1) + 1 produces the same two strings and stays green.

They stop agreeing as soon as a row is removed, and worse than the comment says: getUniqueKey() returns the lowest unused index, so remove i1, add one, and the rows are stored i0, i2, i1 — the key is not even monotonic with the position. The new case drives the removal through onRemoveRow(), the handler the editor's own AJAX calls, and asserts the surviving second row reads Timer 2 while stored under i2.

The docblock did not list the new hook

PluginSequenceFeatureBase carries @api and its class docblock enumerates what a subclass supplies. getRowTitle() was added to the class and not to that list, so an extender reading the contract would not learn it exists. Both overridables are named there now.

Tests

  • NodeFeatureEditorTest::testEveryRowIsNamedAfterWhereItStands() gains the add control: it asserts each feature's own noun on it, and that it does not answer to the same name as a row.
  • NodeFeatureEditorTest::testTheNumberFollowsTheListOnceTheMiddleRowIsGone() builds a three-rung ladder, removes the middle rung through the handler, and asserts i0 reads "Timer 1" and i2 reads "Timer 2".

French for the three new strings, in the base module's own catalog.

Checks

phpcs Drupal and DrupalPractice clean over the whole module, phpstan level 5 [OK] No errors, cspell clean on the changed files, scripts/check-translations.php exit 0 with no missing French and no orphans, and NodeFeatureEditorTest (10 tests) plus OrchestraCmFormTest green against this branch in a lean root.

Merge request reports