Problem/Motivation

A line of a hold is a record with six parts: the slot, the tariff, the quantity, the fields a module put on it, its flags, and the partition of the resource it is confined to. That shape is written out by hand in seven docblocks, in two modules: HoldPassPrimerInterface, LockAnchorScopeProviderInterface, BookingManagerInterface, Placement, RequestBooker, HoldEngine twice, and a subset of it again in yoyaku_placement's PlacementLockAnchorScopes. Nothing checks that those seven agree, and nothing but prose says what a line is.

It is also built by hand. HoldEngine::hold() spends about ninety of its 425 lines turning the loose arrays a caller passes into normalized ones: reading each key, refusing a line with no slot or a quantity below one, refusing a tariff that is not a tariff, and assembling the six parts into a fresh array. That is parsing, and what it parses into is another untyped array, so every reader downstream goes back to string keys and every one of them may be wrong.

The cost is not only readability. Because a line is an array keyed by strings, the engine threads it by reference through assignPartitions() and writes $lines[$index]['partition'] into it, which static analysis can only describe loosely; a whole class of imprecision on that seam exists because the record has no type. The engine reads a line's keys in 44 places.

Proposed resolution

Give the normalized line a type. A small immutable value object holding the six parts, with the validation that hold() does today as the way one is made from a caller's array, and a copy-with method for the one thing that changes after it exists, which is being given a partition.

The loose array stays where a caller passes one: BookingManagerInterface::holdInTransaction() takes what the outside world sends, and the engine parses it once. Everything after that parse is typed, which is the point. The two provider interfaces that receive lines take the type instead of an array, so what a provider is handed is stated by the signature rather than by a paragraph, and yoyaku_placement's two implementations read properties rather than keys.

This is the house idiom already: LineFields, HeldRequest and InventoryControl are all types that replaced an array or a set of arguments. This is the same move on the record those three sit inside.

Remaining tasks

None of this touches the locked section. The normalization runs before any lock is taken, which is what makes this safe to do while #3618870: Move the hold path into an engine of its own, so BookingManager is the contract rather than the implementation's ordering argument still holds: the try block is where the order matters, and nothing there is being reordered.

The provider interfaces are @api, so both implementations in yoyaku_placement move with them in the same change and the old shape is deleted rather than kept alongside.

User interface changes

None.

API changes

PartitionProviderInterface::partitionsFor() and HoldPassPrimerInterface::primeForLines() receive the new type rather than arrays. A module implementing either updates its reads from keys to properties. The engine's own public interface does not change, and what a caller passes to it does not change.

Data model changes

None.

AI-Generated: Yes (Claude Code was used to take the measurements this issue quotes and to help draft this summary. I reviewed it before posting; there is no code on this issue yet.)

Issue fork yoyaku-3618873

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mably created an issue. See original summary.

  • mably committed aa0829e6 on 1.x
    task: #3618873 Give a hold line a type, so its shape is stated once...
mably’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.