Issue #3618881: Name the phases hold() runs before it takes a lock, and say what Drupal says everywhere else

Two things, both about a Drupal developer being able to read this engine.

1. hold() says what it does

It was 443 lines with the locked section buried a hundred and eighty lines down, reading as one more paragraph. It is 256 lines, and reads as four steps and then the transaction:

$normalized  = $this->parseLines($lines, $channel_id);
$giving_up   = $this->linesGivenUp($releasing, $into);
$this->validateBeforeLocking($normalized, $giving_up, $reference, $into);
$anchor_scopes = $this->anchorScopes($normalized, $giving_up);

A pure move, checked as one: all 77 statements are the same text in their new home, the only additions being two returns, nothing removed. Each step's paragraph comment became that method's docblock, where it belonged. All three run before the first lock, so the ordering argument that governs the engine is untouched, and the try block and its catch are not split.

$tenant was the one thing that was not mechanical: a local of the parse loop, used three times inside the locked section. hold() reads it back off the first parsed line, which is sound because refusing a group whose lines disagree on the tenant is what parseLines() is for. phpstan found it, as four Undefined variable.

2. The names are Drupal's, not ours

Every word below was ours where core already had one.

was is why
prime, primer, primed preload* preloadPathAlias() does exactly this; prime is 0 in core
HoldPassPrimerInterface HoldPreloaderInterface and Pass goes: a Drupal developer knows it as CompilerPass
tally() addTakenLine() / addTakenPlace() / addTaken() core records into state with add*; tally is 0 in core
forget() reset(?int $slot_id = NULL) core's own shape: resetCache(?array $ids = NULL), NULL meaning everything
GroupPrimedConstraintInterface LineGroupConstraintInterface "group" alone never said group of what, and Group is a contrib module
parseGroup() / holdGroup() / runGroupPolicies() parseLines() / holdLines() / runLineGroupPolicies()
askTheRules() validateBeforeLocking() validate is core's verb: 53 plain validate() there, 19 here already

Two of them were not caches. LockAnchors::forget() and SlotPlaces::forget() run DELETE FROM yoyaku_lock_anchor and yoyaku_slot_place for one slot. Those are deleteForSlot(int $slotId) — core says delete for deleting rows — and they take no default, because a NULL-means-everything reset() there would empty the table. That is the one place a rename sweep could have caused data loss, and it is why the bodies were read rather than the names.

LineGroupConstraintInterface keeps its three methods deliberately: preload() reads the group once, addTakenLine() counts each line as it is saved so a request naming one seat twice cannot pass twice, and reset() drops everything at the lock boundary. Naming it after preloading would have named a third of it and hidden the two that stop double-booking.

API changes

HoldPassPrimerInterface, GroupPrimedConstraintInterface and BookingClientInterface::holdGroup() are renamed, with their methods and the yoyaku.hold_pass_primer service tag. Every implementation is in-repo and moves in this commit. Free under the pre-1.0 policy.

Testing

phpstan level 3 [OK], phpcs Drupal,DrupalPractice over 1,216 files clean, the whole unit suite at 2,477 tests, and 17 kernel classes covering the hold path, the placement seams, the lock anchors, the API client and the query budgets.

Edited by Frank Mably

Merge request reports

Loading
Loading