🇬🇧United Kingdom @jonathanshaw

Thanks, that's very clear.

The only stretch conceptually is the parent/child settings, but you have that same need in scopes, so it's a wash.

I think that is perhaps the essence of scopes. So scopes seem unavoidable.

With scopes and entity reference editorial staff can handle idiosyncratic of individual events, whereas (even with scopes) multiple fields per host can only be decided by a sitebuilder in advance.

So scopes seems must-have either way, and relatively non-invasive (as only HostEntity has to know about them), whereas multiple fields touches on the data model and maybe other trickier things all over the place.

We can make it a design goal that scopes should support either scenario (without needing to commit to allowing multiple fields).

I'm happy to do a simple PoC for scopes.

Feedback addressed.

I guess what I am getting at is the need for "scopes" in an architectural sense may go away if we did not have the arbitrary one-field-per-host limit.

I think I'm not getting what you're seeing.

The use case I'm imagining is:
- a node type called "event" with an entity reference field "variations" that references registration_variation entities using an IEF
- the registration_variation entities have a registration field so each variation is a host
- staff can make as many variation as the idiosyncracies of an event need
- when someone wants to register for the event, they pick the variation they want and (technically) register for that

The question of scopes arises when staff say things like:
- I want to turn enable/disable all the variations at once
- I want to have one default open/close date I specify once, not having to do it seperately for each variation but still having the option to it seperately sometimes
- my event can take max 10 people in single rooms and max 10 people offsite, but the assembly hall only holds 15. So I need a capacity for the event as a whole that is different to the sum of the capacity of the individual variations.

All of these needs suggest the need for a registration settings to be definable on the level of the parent event (scope), as well as on the level of each variation (host). Which requires some basic API for how to communicate between scope and host and combine settings from scope and host. I have ideas for how this could work fairly easily and flexibly.

I'm not sure how having multiple registration fields on one entity helps. Can you say more?

I also suspect that adding multiple fields and providing ways to target a registration to one or other of the fields is a much bigger change to architecture than allowing HostEntity to have the idea of a scope that it considers for settings as well as its own settings.

$result = AccessResult::allowedIfHasPermissions($account, $permissions, 'OR');
if ($result->isNeutral()) 
  $result = $this->checkEntityUserPermissions($entity, $operation, $account);{

I get what you mean.

Ah, yes. A child access control handler extending this one could modify checkEntityUserPermissions and not necessarily use AccessResult::allowedIfHasPermissions().

it is possible for someone to not have access to settings, broadcast, or view registrations, and still have access to the manage registrations tab. In this case it shows a summary.

Yes. Perhaps you're right that for BC we should keep an operation just for this. It's slightly annoying as it seems a slightly trivial thing to have an operation for.

I lean towards thinking that the operation should be called "view registrations summary" as that is what the operation is actually about. It doesn't really allow you to "manage registrations" in any obvious sense of what it might mean to "manage" a registration; it isn't even considered when considering access to individual registrations, unlike 'view registrations', update registrations', 'administer registrations' etc.

As for the operation name "broadcast" this would seem to imply the "manage settings" (does that exist?) operation would become just "settings" which doesn't seem right.

I considered that but couldn't think of a good single word verb for "manage settings". "configure" isn't awful but has a somewhat different meaning in Drupal.

But even if we have to leave "manage settings" as two words, that doesn't mean "broadcast" isn't better than "manage broadcast" for the operation. But I accept that it might well not be worth changing.

I'd prefer not to change type, so that field data will be preserved. ... I'm just nervous about clone/delete and if people realize they are possibly losing data. So making them change a setting to enable this type of riskier approach, with a warning in the setting description, gives me some comfort.

I can probably do more to test and/or recover if the save fails. But yes, there is the "loss" of data from fields on the old registration type that are not present on the new registration type.

I can make it a setting, no problem.

I was unable to create a 3.3.x branch even after updating the issue to identify that branch. I would love to know what your steps were

The forked repo is basically just an ordinary git repo. Maybe the branch that is originally created is based on the branch specified on the d.o issue. But after that, you can make any other branches you like in the fork, pull in any upstream changes, etc. So I just:
- rebased locally on 3.3.x on my existing existing branch
- then made a new local branch from that (because now I'd rebased the commits on my local branch were different to those already pushed to the fork branch
- pushed my new local branch
- opened a new MR from the new branch in the fork against regitration/3.3.x

I get a "Host entity is not configured for registration" exception if I add a possible host in that state in my subscriber. That needs to be treated like any other "not enabled" state, listed like the others with a cause message, so the user can take action or ignore as they see fit.

Nice catch. Definitely a bug. But I'm not sure about the fix. In a way, the fault is with the event subscriber for adding a "possible host" that is not in any way really a possible host. I wonder if the PossibleHost should throw an exception if you try to construct it withan entity that is not configured to be a host. Although we don't do this with HostEntity so maybe you've got a good reason not to.

evaluate all possible hosts in access control. ... I now believe we cannot do this in access control because we have no idea how many hosts have been added in the subscriber. Making access control that sensitive (potentially hundreds of hosts could be present, each with cache tags, if even one host is updated presumably access control for thousands of registrations would need to recalculate) is bad for performance. So I think we can use the validation results when outputting the possible hosts lists for display, but access control should only look at permissions.

I'm sympathetic to what you say here. I don't have a problem with removing that.

Let's discuss changing the registration type when a host is changed. I had forgotten this was possible - but should it be. I'm not aware of any other places in Drupal where changing type is allowed, because it creates all sorts of problems with fields and revisions (registrations are not revisionable yet but it is a feature request).

The code in question is in RegistrationChangeHostManager::cloneRegistration(). I think I copied it from some contrib module, but I can't find which one now.

The approach used seems fairly safe, it just uses the regular entity API to create a new registration and copes values across. Or rather, it's safe provided that the new registration saves ok; in ChangeHostForm I delete the old one first (to avoid an id collision) so there's trouble if the save on the new one fails. ChangeHostForm wraps the delete/save in a transaction but I've no idea if that actually offers real protection.

https://www.drupal.org/project/convert_bundles → is an attempt to hack the underlying data rather than creating a new entity and deleting the old one. But the internals of that stuff are scary.

A big question with this stuff are how one wants the entity lifecyle hooks to behave. I decided I didn't mind firing a delete hook for the old registration and an insert hook for the new registration. It's sort of like they cancel and instantly register. It's not a perfect choice, but it's maybe not worse than not firing these hooks, and it's much much simpler to use the regular entity API rather than messing with the sql internals.

I'm doubtful that equating field instances and scopes is a good idea. Field instances are configuration, but my sense is that event variations need to be more like content: lightweight, idiosyncratic to an individual event, and created by event staff not sitebuilders. My experience is that individual events very commonly have idiosyncratic requirements that emerge unexpectedly at short notice during routine event preparation.

At first I thought you meant the parent would have a registration field, and a reference field that referenced children which had their own registration field, and a user registering would (behind the scenes) create two registrations, one for parent and one for child.

Hmm. That means one would have multiple registrations for what was effectively 1 event or booking in the minds of registrants and staff. What's good about this is it allows for different registration types and so different fields for the overall event and for particular variations. But it would also mean the underlying data model is going to be quite divergent from the naive user mental model.

jonathanshaw → created an issue.

Perhaps there's a case that if we want to deprecate these, the sooner we do it the better, especially if the path forward is not straightforward for some sites.

It's removing the permissions that has to wait for a major version, not deprecating them.

I think this is best postponed on 📌 Check the administer operation when checking other registration operations Active

I'm OK with A, but I think B is slightly preferrable as it provides more granular extensibility by the access hooks. With B, an access hook could for example be neutral about deletion while allowing overriding settings, rather than having to allow both when allowing administering.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

It's not only for admins.

jonathanshaw → changed the visibility of the branch 2503273-allow-admins-to to hidden.

Sorry for being stubborn, but you got me interested and I think there's a way to fix this in the same issue fork with a rebase and a new branch. Let's see if this works ...

I wonder whether the operation should be called 'update state' to align with the 'update' operation. Not that it matters at all.

I think probably the RegistrationAccesscontrolHandler should call out to the host handler with an 'edit registrations state' operation in case the host has an opinion as well. I like doing this almost always (even with no *host permission from this module) as it makes it easier for sophisticated access control per host using the group module and other custom use cases.

This is now ready for review. It really benefits from all of the work we've done on access and validation, this submodule now doesn't need to know about or try to duplicate the implementation details of registration core.

A few notes ...

1. Memory cache. I still think this is a good idea to avoid repeated invocation of multiple expensive events in a single request. But can be done in a follow-up.

2. Waitlist. Quite how this interacts with waitlist is a tricky question. But that's sufficiently complex that I think postponing consideration to a follow-up is sensible.

3. This submodule will benefit from 📌 HostEntityInterface should implement CacheableDependencyInterface Active but doesn't have to wait for it.

4. A default event subscriber - @jphn.oltman #33

I would like there to be a simple subscriber that is there by default, and easily overridden (we'd put instructions in the README), so that a non-developer can at least see the module in action (and in some cases it might be enough). Perhaps something as simple as putting every entity of the same type and bundle as the existing host into the PossibleHosts list (up to some limit perhaps), and making the first 10 that are enabled for registration visible. I agree that most site builders will need to add their own subscriber that overrides it, but I have to think about the non-developer use case as well - of which there are many based on other support requests I get. I don't see a downside to this as it doesn't create more work for the developer use case and it allows people to try the interface out and make a decision on whether they want to put the work into making it perfect for their site with a custom event subscriber.

I continue to think this is not a good idea. My reasons are:

A. It complicates what I see as the main use case, allowing people to switch between hosts that are parts of the same 'scope'. Our current use case for this is commerce_registration, which can provide an subscriber that automatically makes other variations of the same product be possible hosts; and that works OOTB without needing a developer to get involved. I believe that there is a strong case that I will explain more in a new issue why we need to do something like ✨ Establish host variations as an architectural concept Active in order to better participate in the 'Events ' track of Drupal Starshot, and in a similar way this gets messed up if we have a default subscriber with a completely different logic.

B. Many registerable things have start and end dates, and it makes no sense to offer them as registration targets after their end date. But we don't know about the end date, only the close date, and sites are perfectly able to not use the close date; for example if they use a view to only show future events based on start date they may not have bothered to use a close date or disable past events using status, because they don't list them and so no one finds them. It may not be best practice, but it could also be fairly common. And in this case, we can't safely assume that a host with a null close date is or is not a possible host.

Therefore I suggest we make this a follow-up for a separate discussion if you're still interested.

5. I dislike the button text we've ended up with. "Save Registration & change Graduate seminar" is rather long for a button-text; and of unclear accuracy when changing from Undergraduate seminar to Graduate seminar. However this is a tricky problem and I don't have a great alternative to suggest. I might do "Confirm change" or "Save & confirm" if it was up to me.

6. I think I have taken care of all the review points from your earlier reviews above, apologies if I missed anything.

I think I need to create some follow-ups for some of the issues in #8 as well.

In progress

jonathanshaw → created an issue.

(1) I've implememented AllowedRegistrant as you requested.

I actually prefer
RegistrantUnique
RegistrantAllowed

or even better
RegistrationHasUniqueRegistrant
RegistrationHasAllowedRegistrant

To keep the constraints grouped by their rough type.

(2) There's a certain duplication of logic between RegistrationManager::getRegistratntOptions() and the 2 contraints. But I don't see an easy way to mitigate that; DRY might be overengineering here.

I haven't thought about it enough to be sure. The problem you're describing makes sense.

How about something like:

For links:
Registration not available:
Cause1. Cause2.

For form:
Registration for this event is not available:
Cause1. Cause2.

I don't love the way this does access-like logic in a validation constraint and makes validation depend on the current user, but I think it's right.

I'm slightly surprised that adding it doesn't mean we can't take away code from elsewhere, but I can't see where.

I share your unimpressedness with where I got to with this. And I think there's a fair argument that requiresCapacityCheck() is actually more accurate than getSpacesRequested() because of the unintuitive intricacies of the method.

Looking at this whole issue made me less positive with the whole way that states and capacity and capacity methods are working. It's very difficult to understand the meaning of the different states and their properties, how registrations move between them, and how this affects capacity, and how different methods and extension points are involved. However, it's a super hard problem space and likely impossible to fix with backwards compatability. I assume that the system has evolved over time - waitlist in particular is very tricky to understand.

Worth considering an overhaul for a breaking 4.x, but also likely works well enough as is. That the tests pass is kind of amazing ... but there's plenty of them.

Looks good

Just one thing:

$storage_timezone = new \DateTimeZone(DateTimeItemInterface::STORAGE_TIMEZONE);
      $now = new DrupalDateTime('now', $storage_timezone);

My understanding of best practice here is to use the date time.time service to get a timestamp of the current time.

Otherwise rtbc.

There's a fair number of places where it's not easy to say whether we should use isNew() or isNewToHost(), but I reckon I've got most of them. And missing one won't cause any problems for existing sites, it might only lead to a difficulty in some obscure edge case when using registration_change_host.

This uses Registration::getAdditionalSpacesRequested() so postponed on 📌 Explore alternatives to passing existing registration into hasRoom Active .

I get the idea of a setting that controls whether or not register links/tabs still show even when they are no longer truly registerable.

The "keep showing a register link but take people a page that lists reasons why they can't" approach is not a bad idea, although for many sites registering is such an important call-to-action that relying on a tab for it makes no sense and something like RegistrationLinkFormatter is a better approach, in which case 📌 Show causes with registration link Active opens up a better solution to informing people about why they can't register.

Nonetheless, I can see a case for keeping a setting here. What I'm less sure about is keeping the mechanic that powers the existing setting. But (ab)using the status setting to handle this scenario, we make it harder to know what is means when a host is disabled as we found in 📌 Decide whether 'status' and 'close' validation errors should be shown at same time Active .

It is now technically possible for us to stop relying on the cron/status mechanism and instead use HostEntity::isAvailableForRegistration() in the register route access check, but only if set_and_forget is enabled. This would have the advantage of also removing the register link if the host was full or you had already registered, which is probably what someone also wants if they want to hide register when an host is closed or not yet open.

BC is a headache here. One solution would be to add a new setting and deprecate the old setting with a hook_requirements() block that prevented upgrading if the old setting was enabled.

Interesting. I'm broadly sympathetic to what you're saying.

So we actually have 3 scenarios currently:
- by default, the register links or tabs are always shown
- if the site owner enables set_and_forget then they are shown/hidden based on open/close alone (not capacity)
- a developer could chang the route access and use HostEntity::isAvailableForRegistration()

I will leave this as closed, and have related discussion in 📌 Show causes with registration link Active and 📌 The set_and_forget setting is no longer needed Active .

jonathanshaw → created an issue.

jonathanshaw → created an issue.

Yes, that's right.

I think we need to add the settings entity as a cacheable dependency to make sure that changes to the open/close date are considered; as well as the expiry.

Would be great if you could lead on this one @john.oltman.

With the latest solution in 📌 Refactor registration_waitlist RegistrationValidationEventSubscriber Active I no longer believe this is necessary or desirable. It makes sense to me to swap the plugin class if changing the capacity validation logic, but an event subscriber seems a better fit for a simpler change of messaging on an existing violation.

I removed a test I had also added in 📌 Refactor registration_waitlist RegistrationValidationEventSubscriber Active , and that should remove the test failure here.

I have managed to get working a method Registration::getSpacesRequested().

This basically does what HostEntity::needsCapacityCheck() used to do.

What's good about Registration::getSpacesRequested() is that it will help us solve 📌 Improve maximum spaces validation Active . I think this suggests that it's probably a good API addition.

I've also added new test coverage that I created as part of trying to figure things out here. One of these tests currently fails because it exposes the bug in 📌 Refactor registration_waitlist RegistrationValidationEventSubscriber Active .

I'm still interested in clarifying or improving what is going on when we pass a registration into hasRoom() and friends, but it's an extremely intricate and delicate web, tied up with what really things like active or held mean, and how waitlist works. So I'm not going to do more on that immediately if at all.

There's a catch I was overlooking ... Needs a rethink.

Unexpected test fail, needs more work.

Where I've got to with this is:
(1) The first step is to add a Registration::getAdditionalSpacesRequested() method.
(2) This constraint validators benefit from this I think. It solves 📌 Improve maximum spaces validation Active .
(3) This will fail a waitlist RegistrationAdminTest, but I think that's because somehow it exposes the bug fixed in 📌 Refactor registration_waitlist RegistrationValidationEventSubscriber Active .

If you like this new method, then we should discuss how/whether to deprecate passing the registration into hasRoom() and its (numerous) friends. I fear this would require us to create new methods and deprecate the old ones, because changing parameters would break the interface. However, as we only recently made it possible to swap in custom host handlers, there may be next to no custom implementations in the wild that customise the changed methods so that may be an acceptable risk.

Adding the new method here may be worthwhile even if we don't deprecate passing the registration to hasRoom().

Studying this I've arrived at a conclusion:
(1) The current subscriber may be significantly broken, as shown by the failing test I've added.
(2) If we fix hasRoom() so it obeys its interface and returns a truthful answer instead of always returning true, then all the subscriber needs to do is to enhance the message/code if the waitlist is full.
(3) If the subscribers function is simple like this, then it makes sense as a subscriber and I don't see a particular need for 📌 Waitlist validation subscriber better as swapped plugin class Active .

Additional follow-up issues created:
- 📌 Add a Registration::isNewToHost() method Active
- 📌 Explore alternatives to passing existing registration into hasRoom Active
- 📌 Add a register permissions validation constraint Active
- 📌 Improve maximum spaces validation Active
- 📌 Finalize the HostEntity validation API surface Active

The draft change record needs some work before the release. The only behavior change I'm aware of is that previously validation allowed admins to automatically override custom errors. I don't see that as a good thing, but it is a behavior change.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

isAvailableForRegistration() is now in 3.3.x-dev HEAD.

My bad :( I think we shouldn't have given 'manage own $type registration' at all, we should have given 'administer own $type registration settings'.

->setCause(t($constraint->notOpenYetCause))
RegistrationConstraintBase could instead in its constructor do:

        foreach (get_object_vars($this) as $property => $value) {
            if (is_string($this->$property) && str_ends_with($property, 'Cause')) {
                $this->$property = $this->t($this->property);
            }
        }

Not sure if that's a good idea.
Also might be possible to handle the translation automatically in RegistrationExexcutionContext.

I have slight concerns about HostEntity::validate() and what the API surface should be there, but at this point (given our plans for no immediate release) it seems to me there's an overwhelming advantage to committing this as is and iterating in more targeted follow-ups. I'm confident this is so close to being right that the follow-ups will not be too entangled with each other, and entanglement would be the only argument for further iterating in this MR.

Still reviewing, will look more tomorrow ...

I created the follow-ups:

- 📌 RegistrationAccessControlHandler should rely on RegistratonHostEntityHandler to consider the validation for edit access Active
- 📌 HostEntityInterface should implement CacheableDependencyInterface Active
- 📌 Refactor UniqueRegistrantConstraintValidator Active
- 📌 The set_and_forget setting is no longer needed Active
- 📌 Decide whether 'status' and 'close' validation errors should be shown at same time Active
- 📌 Consider registration validation performance impact Active
- 📌 RegistrationIsOpen constraint should add cache expiry metadata Active
- 📌 Use HostEntity::isAvailableForRegistration in RegisterAccessCheck Active
- 📌 Refactor registration_waitlist RegistrationValidationEventSubscriber Active
- 📌 Enhance the admin override checker service Active
- 📌 Waitlist validation subscriber better as swapped plugin class Active

and a roadmap to figure out which of them need to be done before we release this:
🌱 Roadmap for 3.4 release Active

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

jonathanshaw → created an issue. See original summary → .

jonathanshaw → created an issue.

jonathanshaw → created an issue.