The policy system now has a phases enum and a hosts list on its attribute. Both were added to let one plugin type hold rules that have nothing in common, and both are the wrong shape. This proposes replacing them with a declaration of what a rule needs before it can be answered.

Why phases is wrong

A phase names a moment. But when a rule can be answered is not a moment, it is a consequence of what it reads. A minimum quantity rule, at least four per booking, needs no booker and yet can never run as places are asked for: a basket holding one line always violates it, so enforcing early would refuse the first click. It has to wait until the basket is finished. A maximum spend rule waits until prices are resolved. A rule about unpaid past bookings waits until the booker is named. Those are four different waits, and they do not line up with the two phases the enum offers.

The four policies that exist today happen to fall into two groups, which is what made two phases look sufficient. Imagining the rules a booking engine plausibly wants makes the axes obvious: minimum quantities, quantities in multiples, minimum and maximum stay length, a child rate requiring an adult rate in the same basket, a cap on the share of reduced rates, blackout dates for one rate, membership or certification requirements, refusing a booker who has unpaid invoices or repeated no shows, no second loan until the first is returned, holding the last units back from online sale. They need different things and can be answered at different times, and the two do not correlate.

What to do instead

A policy declares its requirements: the lines it is being asked about, a finished basket, an identified booker, resolved prices. The manager evaluates at each checkpoint and runs the policies whose requirements that checkpoint satisfies. The checkpoints are the hold, the basket handed to checkout, the registration form, the payment step (see #3614531: Nothing checks the constraint policies before payment, so a violating order can be charged and only refused afterwards) and the confirmation, each supplying more than the last.

The context must be lazy. Resolving prices walks the tier, the tariff and the resource through the payment resolver, and resolving the booker touches the order layer. If the context builds those eagerly then every hold pays for work no attached policy asked for, and a place map makes one hold per click. Nothing is resolved unless a policy that needs it is actually attached to something in play.

Stop assuming a tariff

An untiered slot is a first class mode: the engine takes a null tariff and the slot capacity governs, which is how an appointment, a room or a piece of equipment is booked. The current naming ignores that. The policy is called a tier limit, its label says per tariff, and its refusal names a tariff that may not exist. It should be a transaction quantity limit, named for the quantity field it sums, with wording that names neither places nor tariffs, so a tool library reads something true.

The hosts follow from the same point. A policy attaches to a resource type, a resource, a slot, a tariff or a tier, and what it counts is whatever it hangs on: on a slot, that session whatever the tariff; on a tariff, that rate across sessions; on a resource, the whole resource. The slot is missing today, which means an untiered resource cannot express a per session limit at all, only a per resource one.

This also removes a surprise. Attached to a resource, the limit currently caps each tier separately rather than the resource as a whole, inherited from how the per booker slot limit reads at that width. Attachment should be the subject.

Every hold path must reach the checkpoints

Since this makes the manager evaluate policies at explicit checkpoints, it has to settle which callers reach them. Today BookingManager::hold() reserves directly and runs no policies at all; only holdGroup() validates. And although RequestBooker is meant to be the one booking path, a placement controller and the API client call holdGroup() directly, so enforcement survives only because that happens to be the method that enforces. Whether a configured policy is asked must follow from what is attached, never from which method a surface picked. This was reported separately as #3614604: A policy whose plugin is missing is silently skipped, so a limit fails open and moved here, because a design that owns the checkpoints designs it out rather than patching it.

Related

A tier exists to carry a per session quota or price override, and to give a booking something to point at. Generation creates one per tariff per slot regardless, so a season is mostly rows that repeat what the tariff already said. That is an engine question rather than a policy one and is filed separately, but it is the same observation: the tariff and the tier are being treated as the axis everything hangs off, and they are not.

Issue fork yoyaku-3614532

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

  • mably committed 7957b87d on 1.x
    fix: #3614532 Constraint policies should declare what they need rather...
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.