Issue #3618847: Let BookingManager read through the services it already injects, instead of keeping private copies

Two pieces of knowledge the engine kept in private copies now have one owner each. No user-visible change, no string touched.

The effective quota

It was stated four times: BookingConsumption::quotaFor() and CapacityLockAnchorScopes::quotaFor() were byte-identical down to the docblock, BookingManager::limitFor() was the same expression, and BookingManager stated it again inline where it walks a page of slots.

SlotTariffLookup owns it now, in two forms, because the callers genuinely differ in what they hold:

  • quotaFor($slot, $tariff) looks the override up, for the three callers that have to.
  • effectiveQuota($override, $tariff) takes a row already loaded, and holds the rule itself.

The page walk uses the second with the rows it primed in one query, so routing the rule through one owner costs no lookup per tariff per slot. That was the way this change could quietly have made things worse.

What the booking rows have taken

Six places, not the three the issue counted: sumConsuming(), sumConsumingGrouped() and sumConsumingByTariffAndAllotment() in BookingManager, CapacityLockAnchorCounts, and two more in yoyaku_placement. Every one of them needed the same three facts, the booking table, the SUM(quantity) measure and the CONSUMING_STATES filter, and differed only in grouping.

ConsumedQuantities states those three once. bySlot() and ofSlot() serve the engine; consuming() is public so a module bounding a scope of its own narrows and groups the statement rather than restating what a consuming line is, which is what PlacementLockAnchorCounts does with its own columns. PinnedPlaceHold turned out to want a sum the engine already produces, and asks for it instead of building one.

This removes a query rather than adding one. The slot-wide total is the same scan added up, since every line counts against its slot whatever else it names, so an availability page now sums the booking rows once where it summed them twice. ConsumedQuantitiesTest::testOnePageOfSlotsSumsTheBookingRowsOnce() asserts it, and I watched it report 2 against a deliberate restoration of the two-scan shape before trusting it.

The forwarding methods, corrected

The issue named four. Only two were really forwarders: BookingManager::forTariff() and BookingScheduleResolver::consumingLines() each wrapped one call to an injected service under the same name, and both are gone. bookingStorage() and transactionStorage() stay: they have four and two callers, so removing them would repeat a storage name four and two times, which is the duplication this MR exists to remove. I have corrected the issue summary accordingly.

Scope

This is the reuse work the issue title names, and it is complete. Decomposing the class along the seams it leaves is a follow-up rather than part of this, so that the title, the summary and the diff describe the same thing; BookingManager is 2,531 lines here, down from 2,644.

Also

Two sentences in SlotTariffLookup left garbled by an earlier sweep are repaired ("so anything that the slot's override has to look it up", and a return description ending "as the it"), and a services comment calling a slot a session is corrected.

Testing

ConsumedQuantitiesTest is new: the arithmetic of all three groupings, that a line outside every allotment still counts against its tariff, that only consuming states count, and the one-scan query budget.

Green locally before pushing: phpstan level 3 [OK], phpcs Drupal,DrupalPractice over 1,213 files clean, cspell adding no new word, ModuleBoundariesTest 34/34 (which is what placement importing a core class had to satisfy), and 22 kernel classes including every query-budget and cost test in the project.

Merge request reports

Loading
Loading