Issue #3618860: Move availability reading out of BookingManager, so a read does not cost the reader the locking

BookingAvailability owns the read: availability(), availabilityMap(), findAvailability() and consumed(), with the nine private methods only they used. BookingManager is 2,060 lines, from 2,529, and a reader asking how availability is computed no longer opens the file that holds the locking.

BookingManagerInterface and its fifteen methods do not move. The four reads stay on the manager as delegations, so nothing outside knows the reading moved.

What the split actually cut along

Three of the nine private helpers are also asked by the hold path, which checks the same bounds before it takes a lock: limitFor(), allotmentFor() and withheldOn() are public on the reader rather than duplicated on both sides. The rest were reachable only from the four reads, and the cluster calls nothing back into the manager, which is what made this a move rather than a rewrite.

Two dependencies left the manager with the reading: the yoyaku.availability_bound providers now feed the reader, which is what folds them, and the consuming sum is read by the reader rather than by the engine. The manager's constructor is 24 arguments, from 25.

The reader memoizes nothing, deliberately. The lookups it folds already cache per request and know when to forget, and the hold path re-reads these numbers under its locks in order to see rows committed since. A cache of its own would answer that second read with the first one's answer, which is the one thing it must never do.

Nothing was lost in the move

The move was scripted, so it was checked rather than trusted: every one of the 54 methods in the previous file exists in exactly one of the two now (45 in the manager, 14 in the reader, the four delegations in both), and no constant, property or import was dropped. That check earned itself: the first pass silently swallowed two properties, $constraints and $consumption, that sat between two moved methods, and phpstan is what caught it.

The second half of the issue is not sound, and I have corrected the summary

The issue proposed extracting the capacity accounting as well, for another ~296 lines. The call graph says that is not a thing the engine has: anchorScopes(), scopesBounding(), startCounting(), tally(), countedOnSlot(), heldOn(), lockRow() and forgetReads() are each called by exactly one caller, all of them in the hold path, and three of them mutate $this->consumption, whose entire purpose is that what was read above the locks never answers below them. Extracting those would put indirection through the most safety-critical code in the module to save 250 lines. So the estimate of ~1,600 in the summary was wrong, and this MR is the part that stands up.

Testing

phpstan level 3 [OK], phpcs Drupal,DrupalPractice over 1,214 files clean, and 22 kernel classes including every query-budget and cost test in the project. The QueryCount budgets pinned on real pages are what would catch a read that started costing a lookup per call; they are unchanged.

Merge request reports

Loading
Loading