Establish host variations as an architectural concept

Thanks for this excellent post @jonathanshaw. Abstracting the host entity further could also help with a number of other feature requests besides the "change host entity" request:

* https://www.drupal.org/project/registration/issues/2587903 ✨ Option to make a registration selection Active
* https://www.drupal.org/project/registration/issues/2634340 ✨ Support multiple capacities by type Active
* https://www.drupal.org/project/commerce_registration/issues/3415425 💬 Multi-Level Pricing? Active

In all three of those requests, there is a need to have the host entity context be different, and at a "grouping" level (i.e. product in a Commerce install), vs. where the registration field is attached, at least for some checks like capacity. You are definitely onto something here. One issue is the context will be totally variable per site - for many, the "event" will be at the variation level and not the product - for others (sounds like your site) the context is at the product level. Certainly in the scenarios in those three feature requests, some host entity checks need to be at the grouping level - maybe all - not sure yet. Going to ponder this further.

My basic thought on the 3 issues you reference is: the challenges we face here are very similar to those commerce faces with products and where they have huge real world experience; leading them to the products, variations, attributes architecture. I'm particularly thinking of the way variations can be auto-generated from combinations of attributes, being the atomic unit to define the customer choice although not always customer-facing.

Maybe we need the same basic setup:
- the atomic host variation
- the grouping that holds a set of atomic hosts
- a range of services to get availability etc that can be decorated/overridden like commerce's price checker etc services, because we know our simple 2 level architecture won't suit every use case.

I realised there's an important distinction between shopping and registering. A person can buy as many different things as they like, and as many of each thing; but a person can only be in one place at a time. Thus there are hosts a user should not be able to register for if they have already registered for another 'host' within the same 'scope'.

This suggests we do have a clear need and meaning for a concept of 'scope' that is higher-level than host. Different sites might attach different meanings to the idea of scope just as they do to host, but the one we insist on is that there can't be multiple registrations of the same user within a single scope.

I haven't even tried to run this PoC code, but it shows how I think of this as working.

A top level overview of how this works ...

- A Scope is a context for a registration. It knows about other scopes it falls within, and hosts that fall within it.

- A scope knows about the registrations that fall within it, because it knows about the hosts that fall within it.

- A Scope can fall within multiple scopes. Consider a complex example. There's an event that has a fixed total quanitty, that varies on various attributes: it has ticket tiers (premium, ordinary) each with a fixed quantity available and accomodation (single, shared) each with a fixed quantity. Those attributes combine to make 4 product variations: premium single, ordinary single, premium shared, ordinary shared, each with a different price. In our example, the premium single product variation falls under 3 scopes: the event, the premium attribute, and the single attribute. Therefore it is only available if all 3 of those scopes have remaining capacity.

- A ScopeEntity is a kind of scope that wraps a Drupal entity, like most scopes will.

- A HostEntity is is kind of scope itself, and so extends ScopeEntity. It's special in 2 ways: (1) it also implements HostEntityInterface so it's possible to actually register for it. (2) It knows that the only host it needs to concern itself about is itself.

- ScopeInterface adds new methods that abstract settings: isEnabled(), getMaximumSpaces, isMultiplRegistrationAllowed(). These get used in validation rather than directly querying host settings.

- Scope methods can be divided into 2 basic kinds:
(1) those that are look up to consider other scopes they fall within, and ins some sense combine settings. For example, getCloseTime() returns the earliest close time of this scope or any scope this scope falls within. Similarly for getOpenTime, isBeforeOpen, isAfterClose, isEnabled, hasRoom.
(2) those that look down to consider all hosts that fall within them. This is mostly getRegistrationQuery() and everything that uses it to consider existing registrations, like isUserRegistrant and getSpacesReserved. But also isAvailableForRegistration().

If you get a chance to refresh the MR branch now that change host is committed, that would help me evaluate your PoC. Thanks!

OK, I've refreshed the branch, and got the code to actually work enough to pass the existing tests.

Something is off because the change host updates are still showing in the MR as changes, which makes it hard for me to see what the real work is. We may need a new branch fresh off the 3.3.x tip.

Can you recheck that? It's not happening for me in the Gitlab GUI when I look at the latest !103.

It's easy to be unintentionally looking at an older commit in the Gitlab GUI

I think I just needed to hard refresh my browser - I can see things properly now. If you are going to do more work on this, please start a branch off 3.3.x and apply your changes there, it looks like you used an unmerged work branch from the change host work as the starting point, based on the commit history of the MR. I know this is only a PoC but if you do more on this might as well level up.

As for the work itself - I am convinced of several things after seeing this:

* Multiple field support is not the right way to address our need for scopes - what you have here, at least directionally, is the way to go.
* I still want to allow multiple fields, but they would be separate and not communicating - like two text fields on an entity - they would share a host entity, so related in that sense, but separate in every other sense, as two different fields should be.
* This scope work, and any possible multiple field work, should probably be a totally new version - like 4.x - not deprecating, but starting over. Perhaps a migration path - but not like going from 3.3 to 3.4. A complete BC break. Not wedded to this but feels like we need it. I know core deprecates from 10.x to 11.x for example, but this feels like more of a rewrite.
* Let's tidy up 3.4 work and get that release out sooner rather than later. I think we are very close already.
* I want to prioritize QR codes and ticketing in 3.x before attacking scopes - of course you are free to continue scope work as you see fit.
* I think we would need to work more closely on this - meet on Slack or something - to collaborate on how scopes would work in the registration UI etc. I like this PoC, but it raises many questions, things we would want to agree on before work started in earnest. Also at an API level having both a Scope and ScopeEntity I find confusing, maybe it's correct but those are the types of things we should iron out before getting too far along.

Let me know your thoughts. Thanks again for this!

This scope work, and any possible multiple field work, should probably be a totally new version - like 4.x - not deprecating, but starting over. Perhaps a migration path - but not like going from 3.3 to 3.4. A complete BC break. Not wedded to this but feels like we need it. ... this feels like more of a rewrite.

My sense at the moment is that it is actually surprisingly backwards compatible. The HostEntity grows in what it considers, if you give it a scope. But by default, it doesn't have any scope so its behavior is unchanged.

The reason why there's quite a lot of deprecation in the MR here is because
(a) all of the existing events expected a 'host_entity' array key to be passed to them and in order change this to 'scope' I deprecated, but I could equally have just passed 'scope' as well as 'host_entity'; and
(b) once I started deprecating events I got greedy and reworked a chunk of the room/space/capacity logic that I found weird but which is hard to change without deprecating events

You could fairly accuse me of gratuitous scope creep.

Where it will get interesting with BC and new versions is in other aspects of ✨ A plan for Drupal CMS support Active like a new variation entity type and changes to IEF support to play nicely with that. But even there I suspect we might mostly need to change the defaults for new installs, not mess with existing installs.

Let's tidy up 3.4 work and get that release out sooner rather than later. I think we are very close already.

Agreed. It's falling into place nicely.

I want to prioritize QR codes and ticketing in 3.x before attacking scopes - of course you are free to continue scope work as you see fit.

Sadly I shouldn't really let myself spend too much time playing with this. I think good Starshot support is valuable for the adoption of this module, and I'm happy to discuss, but I need to rein myself in.

Sadly I shouldn't really let myself spend too much time playing with this. I think good Starshot support is valuable for the adoption of this module, and I'm happy to discuss, but I need to rein myself in.

I totally get it. You have a life and a real job to get to I'm sure. I will ping you on Slack to see if we can discuss while things are fresh. Then I can circle back later this year with more thoughts. I can also get 3.4 over the finish line on my own from where things sit.

Thanks again for your contributions and I hope to see you opining on various issues from time to time.

john.oltman → closed merge request !103

Something that did come up in this MR which may be 3.4 relevant is that $host_entity->getSetting('status') or $host_entity->getSetting('capacity')is not the best design pattern. We need to move towards $host_entity->isEnabled() and $host_entity->getCapacity(). In some sense the settings storage is implementation we don't need to expose. This allows the host entity to do more creative things, like consider scopes.

I mention this now, because we use things like $host_entity->getSetting('status') in the new constraint validators, and I'm wondering whether it would ease our future BC if we addressed it before releasing the validators. I'm not sure it does, but I wanted to flag the question as BC can be such a pain.

One thing is definitely true though: if we added these getters to host entity, we wouldn't need the dependency mechanism in the validator and constraints, which would be a simplification win.

I've rebuilt the MR, hopefully clean this time.

john.oltman → changed the visibility of the branch 3419239-establish-host-variations to hidden.

Thanks, MR looks much better.

In some sense the settings storage is implementation we don't need to expose.

I am following the design pattern in field formatters in core. Like with formatters, site builders can add their own settings. That shouldn't prevent any creativity with scopes, I wouldn't think.

If we added these getters to host entity, we wouldn't need the dependency mechanism in the validator and constraints.

The dependency mechanism is there not just for the settings, but to take care of adding the cacheability. Otherwise every validator would need to repeat its own code to add cacheability for the host entity and the settings entity. So in my view the dependencies are simplifying things.

The dependency mechanism is there not just for the settings, but to take care of adding the cacheability. Otherwise every validator would need to repeat its own code to add cacheability for the host entity and the settings entity.

A good consideration. But 📌 HostEntityInterface should implement CacheableDependencyInterface Active is something we should probably do anyway because we have the same need in other circumstances, and it would be a more conventional and no more complex DX if we used it in constraints, without the maintenance burden of the validator dependencies mechanism. Not important, just an observation.

is something we should probably do anyway because we have the same need in other circumstances

This makes sense. I'll explore 📌 HostEntityInterface should implement CacheableDependencyInterface Active further.

Actually even if we do 📌 HostEntityInterface should implement CacheableDependencyInterface Active , without the dependency mechanism, each validator would have to add the cacheability using the same code as in other validators, since no validator could rely on some other validator to do it first. So it doesn't seem to help with that. I'm also not sure I want the validators adding cacheability to the value being validated. It should be up to the caller to decide what to do with the cacheabiliity info added to the result. This is similar to $host_entity->access(operation) - the cacheability is added to the access result, not the host entity. Similarly the cacheability should be added to the validation result and not the host entity. Access results and validation results have symmetry in that way, intentionally.

I amended my previous answer as I think I misunderstood what you wanted to do in 📌 HostEntityInterface should implement CacheableDependencyInterface Active .

Basically, if we do what you suggest there, we can do this type of thing in constraint validators and access control:

$this->context->getCacheableMetadata()->addCacheableDependency($host_entity);

instead of having to add individual dependencies on the host entity's real entity, and the settings. And extenders of HostEntity can easily add more to it, and all the existing code would pick those additional dependencies up automatically.

Seems like a nice clean approach. I don't think it helps get rid of the dependency mechanism, but better architecture for sure.

$this->context->getCacheableMetadata()->addCacheableDependency($host_entity);

instead of having to add individual dependencies on the host entity's real entity, and the settings. And extenders of HostEntity can easily add more to it, and all the existing code would pick those additional dependencies up automatically.

Yes, exactly.

I don't think it helps get rid of the constraint dependency mechanism,

I'm not sure why we need the constraint dependency mechanism if we have this. The validator knows what it uses, and adds the appropriate cacheability. Maybe someone has a host entity class that has a custom implementation of hasRoom() that doesn't need the settings entity at all, so hardcoding the dependency into the constraint is actually unhelpful. And having every validator have to add the host entity as a cacheable dependency if the validator calls a host entity method, is repetitive but it's only one line and a nicely explicit. Maybe I'm missing something bigger.

Yes, it is much bigger. Every validator would have to duplicate the same logic as the HostHasSettings validator - all of it, including adding the violations, since no validator would be able to assume that some other validator already added those errors. It isn't one line of cacheability - it's 50 lines of code. Also, if someone is extending the host entity class, also overriding the HostHasSettings validator to not add the settings entity to cacheability probably isn't going to be a big challenge for them.