Problem/Motivation

HoldEngine::hold() was 443 lines, 169 of them code, written as one unbroken run. Read by what it does it is four things and then the locked section: parse the lines and confirm the group may be held, work out which held lines are being given up, ask every rule about it before any lock, work out the rows it will wait on. Three of those four had no name, so the try block that matters most read as one more paragraph a hundred and eighty lines down.

The second half is vocabulary. Much of this engine is named in words this project invented where Drupal core already had one, so a contributor arriving has to learn a vocabulary for things they already know. An audit of every method-name verb against core found the inventions: prime, primer and primed (about seventy occurrences, and zero in core), tally (zero in core), and forget, where core says reset. Class suffixes were already core's and needed nothing.

Proposed resolution

Name the three phases, as private methods on the same class. Nothing leaves HoldEngine and the class does not shrink: what changes is that hold() reads as four steps and then the transaction, and each phase's paragraph comment becomes its docblock. All three run before the first lock, which is what makes it safe; the try block and its catch are not split.

Then say what Drupal says. preload for reading a set up front, as preloadPathAlias() does. add for recording into running state, as addCacheableDependency() and addConstraint() do. reset for dropping what was read, in core's own shape: one reset(?int $slot_id = NULL) where NULL means everything, rather than a scoped method beside a wholesale one.

Two of the methods called forget() are not caches at all: they run DELETE against a table for one slot. Those become deleteForSlot(), with no default, because core says delete for deleting rows and because a NULL-means-everything reset() there would empty the table.

GroupPrimedConstraintInterface becomes LineGroupConstraintInterface. Group alone never said group of what, and Group is a contrib module a Drupal developer already knows as groups of users. The interface has three moments, only one of which preloads: the other two count each line as it is saved, so a request naming one seat twice cannot pass twice, and drop everything at the lock boundary. Naming it after preloading would have named a third of it.

Remaining tasks

Behaviour is identical, which for the extraction means the statements move without being rewritten, and for the renames means no signature changes but the names. Two @api interfaces and a service tag are renamed; every implementation is in this project and moves with them.

User interface changes

None.

API changes

HoldPassPrimerInterface becomes HoldPreloaderInterface and GroupPrimedConstraintInterface becomes LineGroupConstraintInterface, with the methods they declare and the yoyaku.hold_pass_primer tag. BookingClientInterface::holdGroup() becomes holdLines(). A module implementing any of them renames its methods to match.

Data model changes

None.

AI-Generated: Yes (Claude Code was used to run the vocabulary audit this issue quotes, to help draft this summary, and to write the code on the merge request. The linters and the named test classes were run before it was pushed; a maintainer reviews it before it merges.)

Issue fork yoyaku-3618881

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’s picture

Status: Active » Needs review
mably’s picture

Title: Name the phases hold() runs before it takes a lock, so the locked section reads as the subject » Name the phases hold() runs before it takes a lock, and say what Drupal says everywhere else
Issue summary: View changes

  • mably committed 27c64bdb on 1.x
    task: #3618881 Name the phases hold() runs before it takes a lock, and...
mably’s picture

Status: Needs review » 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.