Allow admins to change host entity for existing registration

Hi, I would really appreciate this feature.

I have working code for this, as a new submodule of registration, I will open an MR here in the next few weeks.

@jonathanshaw: Great, thanks you!

So, how is it, does it work in v. 3.1.1?

Sorry this has been slow, but it really is coming. Should be doing the final touches very soon.

I think it's worth considering ✨ Establish host variations as an architectural concept Active as it makes this easier.

Chiming in - in my view, the candidate list for the new host entity should be any host entity matching the registration type of the registration that wants to change its host - it does not need to be limited to just those hosts for the same "event". For example, in a Commerce context, I do not think it needs to be limited to only those product variations that are "siblings" (attached to the same product as the original host entity). In the real world, people often want to change from one "event" to another - for example, I have an appointment for a class, and want to switch classes but keep the other registration data intact.

Created a new MR with a new submodule to change the host entity.

Thanks Victor for your contribution! I tried this out on a local test system - from what I can tell, in order to engage your module, you have to write an event subscriber in a custom module to identify which host entities can be used as the new host - is that accurate? I can give more feedback but wanted to start with that question. (I like the idea of using an event for this BTW.)

In order to engage your module, you have to write an event subscriber in a custom module to identify which host entities can be used as the new host - is that accurate?

Yes. I had been concerned that showing all hosts would be a bad choice in almost all use cases, although now that I think of it I realise that showing all hosts open for registration currently could be alright for staff in many use cases, although again unlikely to be adequate for users in most use cases.

I plan to provide an event subscriber allowing to change to other variations of the same product for commerce_registration, which is the main use case I've been thinking of.

Re #12 I definitely think we should *allow* changing host without hardcoded restrictions. But I'm expecting that every site would need to make it's own decisions about which changes to allow based on it's business logic - I don't see how we can really help with this. Hence the "highly flexible but completely opt in and code based" approach of the MR.

Thanks jonathan, didn't realize you and Viktor are working together, nice. Some great work here. Issues with the current MR:

* tests are failing and phpstan is reporting a warning
* after enabling the module I lost my ability to do a regular registration edit - needs to be some way to trigger the change host action - perhaps through a "Change Host" action button at the bottom of the View Registration page (see registration_workflow_preprocess_registration for example) or through a new "Change Host" tab for the registration, or both. The route should have a custom access checker that disables the route for the given registration unless there are at least 2 possible hosts. If you don't provide a custom event subscriber to provide possible hosts, then the admin UI experience should be as though the module were not enabled.
* The paths and verbiage should be more clear about what is happening. "Select registration type" (/type) should probably be "Select new host" (/change-host) and "Update registration" could be "Confirm and save new host" (/change-host/confirm) or something like that. Also, if the registration type isn't changing, then there is no need to launch step 2 for the field updates and it could simply be a confirmation page "You are about to change the host from X to Y" with Confirm and Cancel buttons.
* The selection interface for the new host is really neat - but it needs to exclude any possible host that is not configured for registration - and if too many possible hosts are returned, maybe it needs to have a pager (i could accept without a pager for now).

Setting this issue to "Needs work". If you need assistance with the legwork on the above, let me know.

Also - probably need a separate permission for changing host. Maybe "administer registration" gets access automatically, and then a new permission that can be added to roles for non-admins. On some sites, I could see a use case where some people can edit registrations and others can change the host but not edit.

Sorry for the delay here, I will get Victor to make these fixes.

The paths and verbiage should be more clear about what is happening. "Select registration type" (/type) should probably be "Select new host" (/change-host) and "Update registration" could be "Confirm and save new host" (/change-host/confirm) or something like that.

This makes "host" a user-facing concept. But its meaning is completely opaque to a site's user, certainly to a non-staff user.
I think hosts is ok for the path - people aren't surprised by weird urls.

Ideally for on-page text maybe we should use the label of the host bundle. But in practice this seems tricky as there could be multiple valid host entity bundles or even entity types. I suppose we could use the bundle label if all available hosts are from the same bundle, the entity type label if they're all of the same type but different bundles, or "host" if there are multiple types involved? That's probably the best UX.

The selection interface for the new host is really neat - but it needs to exclude any possible host that is not configured for registration

I've asked Victor to fix this in PossibleHostEntity::access(). This way event subscribers can return hosts to be listed, but displayed as unavailable. I'm imaging that some sites might sometimes want to list a certain option without making it available (e.g. marking it sold out) rather than simply hide the option altogether. template_preprocess_registration_change_host_list() only converts a list item to a link if its access is allowed.

Updated the MR.

Currently, there are warnings in cspell job but it's not related to the current issue.
There are also errors in tests but none of them are related to our submodule.

e.g.

Drupal\Tests\registration\Functional\RegistrationFieldTest::testRegistrationFieldAssignTypePermission
ElementNotFoundException: Button with id|name|label|value "Save and continue" not found.

FYI I had the same error for another project and fixed it by creating fields in another way described here - Button values and messages during field creation workflow have changed →

thanks

Awesome, looking forward to trying this out again. Will review within the next few days.

This is pretty ready for review, although I've asked Victor to tweak the hook_entity_access a bit.

Yeah, I see what you mean - the entity access hook is not setting the cache dependencies. The function "node_node_access" in core shows roughly what has to be done - caching either per user (for "own") or per permissions, and adding cache dependencies on both the registration and the existing host entity for the registration. Also, it should return forbidden unless the host can actually be changed (> 1 possible host), even if the user would otherwise have access. No sense in showing the Change Host tab for no reason. In my view the change host manager should actually remove the existing host if it's in the list returned from the event - that way you can just see if the possible hosts is empty in the access check.

Everything else is looking really good. Setting to needs work for these last tweaks. Thanks!

Also, it should return forbidden unless the host can actually be changed (> 1 possible host), even if the user would otherwise have access.

I believe Neutral is equivalent to Forbidden for route access, but Neutral would allow RegistrationAccessControlHandler to override and grant access if the user has administer registrations permissions, so I think that will be the better approach to DRY.

quick update: currently, I’m busy with some other things. so, will continue working on the issue in one or two weeks

thanks for understanding

No problem, thanks for the update Viktor

OK, finally this ready for review again.

The coding standards failures on CI are unrelated.

The one piece I am aware is missing is related to the event dispatching. The event can be used in many different places on a request: checking route access, checking entity access, building render array, adding cacheability to render array. Currently this leads to the event being dispatched multiple times on a single request. It got too complex trying to hack ways to avoid this so I ended up accepting it.

I'm not sure it's a significant performance issue. But if we do care about it, the best solution would be to implement a memory cache in RegistrationChangeHostManager and cache the dispatched event there. I've held off from implementing this (it's a bit esoteric) until the the rest of the code was agreed.

In #19 @john.oltman said:

The paths and verbiage should be more clear about what is happening. "Select registration type" (/type) should probably be "Select new host" (/change-host)

The path is now "/host".
The title is "Select registration for" because I'm sceptical of making "host" a user facing concept as explained in #21.

and "Update registration" could be "Confirm and save new host" (/change-host/confirm) or something like that. Also, if the registration type isn't changing, then there is no need to launch step 2 for the field updates and it could simply be a confirmation page "You are about to change the host from X to Y" with Confirm and Cancel buttons.

I didn't implement this. I'm not sure the proposed title is better, this form is identical to that for updating the registration, and does in fact update the registration, so having the same title seems sensible to me. This is more obvious for registration types that have many fields.

I see giving a chance to edit registration info at the same time as changing host to be a UX feature, not a bug (even when the new host is of the same bundle so this is not technically necessary). It forces people to reconsider all of the registration info when they change the host, which they probably should be. Changing the host is a significant change in the registration, typically one of the biggest changes possible, and I'd imagine it will very often require at least reconsidering answers given to other questions about the registration.

It's true that if the new registration type has no fields, then the UI here is a bit sparse. I propose we add some text to the page like "This was a registration for ... You are updating it to be a registration for ..."

Thanks for your latest @jonathan I am still testing but wanted to provide some status.

CI errors - I merged the 3.1.x branch back in to resolve
Access bug - I noticed an issue when flipping hosts between enabled for registration and disabled, so pushed up a fix
Admin routes - the change host routes should be admin so users with access to the admin theme have a consistent experience - pushed up a commit for that

I'll get back with more feedback soon. Overall looking good.

Getting closer. More feedback.

I see giving a chance to edit registration info at the same time as changing host to be a UX feature, not a bug... Changing the host is a significant change in the registration, typically one of the biggest changes possible, and I'd imagine it will very often require at least reconsidering answers given to other questions about the registration.

This makes sense and I'm on board with sending people through the update screen all the time, provided there are some minor UX changes per below.

I'm sceptical of making "host" a user facing concept as explained in #21.

I am not on board with this, because the point of the new submodule is to make "host" a user facing concept so site admins can change it, and the admin clicks on a tab saying "Change host" to initiate the process. So "host" is front and center and pretending it isn't user facing is making the UX more difficult than it needs to be. I have some other changes to recommend as well so I'll post all in one new comment.

Recommended changes below.

UX - Right now it is unclear what the state is after selecting a new host and getting to the update page. Has the new host been applied and saved already? Why am I on an update page - did I click the Edit tab by mistake? If I close my browser at this point is the new host set or lost? Recommended changes: Page 1 title is "Select new host", Page 2 title is "Confirm new host and registration data" and page 2 indicates the previous host and new host somehow (and that you are "about to change" to a new host but it hasn't happened yet). I do not think "Delete" should be a link on page 2 and the button text could be "Save Registration and Set New Host" to make clear that unless the button is pressed nothing has changed yet.

Existing host in possible hosts list - The existing host is always present in the displayed list - either hide it on the display side or add "(existing host)" after the title and make it not selectable. Otherwise it is confusing why it is there and people may wonder "do I have two entities with the same title?"

Possible hosts access vs. display - I think we may need to distinguish between possible hosts used for cacheability (in the list so if they become enabled for registration the Change hosts tab shows up) and possible hosts that are visible to the user. Perhaps add a new method "setVisible" on the PossibleHost object, only hosts that are visible are displayed - selectable if enabled for registration and not selectable if not enabled yet. If there are zero visible enabled hosts, the Change host tab is hidden.

Change host entity operation - this does not update like the Change host tab does, when hosts move between enabled and disabled for registration, due to a bug in Drupal core with entity operation cacheability. So if a possible new host becomes disabled and there are no longer any available hosts, clicking on it gives a 403. So I recommend we remove it until the core bug 🐛 Views entity operations lack cacheability support, resulting in incorrect dropbuttons Needs work is fixed in D11. (I realize I am the one who asked for this in the first place, my apologies, did not realize the core bug would affect this.)

Lack of a default event subscriber - 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.

README - We'll need this eventually once everything else is set.

Thanks Jonathan for all your work and as I have said before, I can do the legwork on anything you are not comfortable with or do not have time for.

I am not on board with this, because the point of the new submodule is to make "host" a user facing concept so site admins can change it, and the admin clicks on a tab saying "Change host" to initiate the process.

I'm personally happy to go along with 'host' as it doesn't touch on anything that concerns my project. But there's a few nuances here I think it's worth you considering:
(1) It's perfectly reasonable for sites to allow end users to change host; especially in something like commerce_registration where variations are the 'host' and allow different types of regisration for a single event. Naming matters a lot in this use case.
(2) Most uses of registration I imagine will happen in the context of some kind of event. And in the world of events, a "host" naturally means the person who is hosting the event. Using it in this very different way risks confusing even admins.

A step towards a solution would be to add a getPossibleHostLabel($registration) method to RegistrationChangeHostManager allowing anyone decorating that service to override the default 'host' and use their own approach to providing UI text here.

A further step would be to add a host_label setting to RegistrationItem allowing sitebuilders to configure the UI text to use in this context, defaulting to the name of the bundle the field is configured on.

And it'd not be unreasonable for you to think I'm over-engineering an edge case ...

Good points Jonathan and not over-engineering - on the View Registration page the name of the host entity bundle is used. We could probably use that same label everywhere I am proposing to use "host". So the tab would read "Change Event" if the host entity type is an Event content type and page 1 title would be "Select new Event" etc. The relevant function is right on the Registration entity with method name "getHostEntityTypeLabel" and it returns the bundle label if the type has bundles, so "Event" in my example. If you can give that a shot in your changes please do, otherwise I'm happy to add this when you are done with your other work.

My #34 cross-posted with your #33, thanks for looking so carefully!

Agreed in principle on all except:

Page 1 title is "Select new host"

Existing host in possible hosts list - The existing host is always present in the displayed list - either hide it on the display side or add "(existing host)" after the title and make it not selectable. Otherwise it is confusing why it is there and people may wonder "do I have two entities with the same title?"

A complication here is that I'm trying to make it easy to implement my preferred non-standard workflow where the user doesn't choose between "edit" and change host" but instead "edit" always takes them through the select host page, i.e. a single 2-step editing process.
I'd prefer "Select {host_type_label}" to "Select new {host_type_label}" for that reason, and I think that works well for either workflow.

Regarding the existing host being in the list, I think you're right, but I'd like to implement in a way that makes it easy to put the current host back in the list for the workflow where that is desired. I think having it in the list but not selectable is my preferred solution probably.

Possible hosts access vs. display - I think we may need to distinguish between possible hosts used for cacheability (in the list so if they become enabled for registration the Change hosts tab shows up) and possible hosts that are visible to the user.

I suspect this is a bigger issue actually. The registration settings data is really like a part of the host entity data and should be treated like that for cacheability purposes. Therefore I think RegistrationSettings::invalidateTagsOnSave() should be more aggressive. It should invalidate the list cache tags for the host entity. If we did this, the possible hosts cacheability would work fine as long as the subscriber added the appropriate list cache tag.

I'd prefer "Select {host_type_label}" to "Select new {host_type_label}" for that reason, and I think that works well for either workflow.

Sounds good, I'm on board.

It should invalidate the list cache tags for the host entity. If we did this, the possible hosts cacheability would work fine as long as the subscriber added the appropriate list cache tag.

Good idea, but can we use [registration_settings:list] instead of the host entity list. I'm concerned about forcing a rebuild of every node list block on the site when settings for one event are updated. Sites that want to couple the caching for hosts and settings can use the registration_inline_entity_form module to achieve that, or do custom code. I'm afraid to force it for all sites at this point given there are hundreds of existing installs.

Possible hosts access vs. display - I think we may need to distinguish between possible hosts used for cacheability (in the list so if they become enabled for registration the Change hosts tab shows up) and possible hosts that are visible to the user. Perhaps add a new method "setVisible" on the PossibleHost object, only hosts that are visible are displayed - selectable if enabled for registration and not selectable if not enabled yet. If there are zero visible enabled hosts, the Change host tab is hidden.

On further thought, I'm not clear on the need here. It's true that if a developer wants to only have hosts configured present in the list then in their event subscriber they need to load their possible host entities and filter by whether they are enabled for registration. But they could (and should) add the registration settings cache tags of any disabled host to the event at that point, ensuring that the possible hosts list is rebuilt if any of the disabled hosts get enabled.

This is fiddly, but I think that's unavoidable really. We could add a protected helper method on the event class (removeDisabledHosts()) that provides the 'best practice' logic for handling this.

On further thought, I'm not clear on the need here. It's true that if a developer wants to only have hosts configured present in the list then in their event subscriber they need to load their possible host entities and filter by whether they are enabled for registration. But they could (and should) add the registration settings cache tags of any disabled host to the event at that point, ensuring that the possible hosts list is rebuilt if any of the disabled hosts get enabled.

Wouldn't having the subscriber add a cache tag of [registration_settings:list] be all that is needed? Then any settings update triggers a rebuild of the possible hosts list/display. I thought this was your idea actually. Then the subscriber only needs to add the hosts it wants to display to the list.

Wouldn't having the subscriber add a cache tag of ['registration_settings_list'] be all that is needed? Then any settings update triggers a rebuild of the possible hosts list/display. I thought this was your idea actually. Then the subscriber only needs to add the hosts it wants to display to the list.

You're right, that works for the cacheability, although it will invalidate more often in some circumstances because it will respond to changes to any host, including ones not even considered by the subscriber. Not that this is important.

But this use case of needing to include only hosts enabled for registration is likely very common. So there may be merit to a helper anyway. And if we do have one, we might as well use it to add the cache tag for excluded hosts, so the subscriber doesn't have to remember the list tag and doesn't get invalidated so often. Maybe we should add as our helper an addHostIfEnabled(PossibleHostEntityInterface $host) method to RegistrationChangeHostPossibleHostsEvent in addition to addHost(PossibleHostEntityInterface $host).

Maybe we should add as our helper an addHostIfEnabled(PossibleHostEntityInterface $host) method to RegistrationChangeHostPossibleHostsEvent in addition to addHost(PossibleHostEntityInterface $host).

I like it, sounds like a plan. Please proceed (and thank you).

@jonathanshaw I am changing this to Needs Work. I saw an initial flurry of commits, but I am guessing there is a bit more to do. If I am wrong, please put this back to Needs Review and assign back to me. Thank you for all of your efforts.

Yes, sorry I stalled but plan to finish,!

This is substantially finished. Todo:
- fix functional test failures
- coding standards etc
- memory cache to avoid dispatching the event multiple times

And keep up with possible changes in 📌 Replace HostEntity::isEnabledForRegistration Active :
- change access and test logic if we use a registrant permission constraint
- use HostEntity::validateRegistrationForHost() if we adopt it

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.

Looking forward to reviewing this. Unfortunately this was forked from 3.1.x and GitLab does not yet allow a second fork on the same issue, so I created ✨ Add the ability to change the host entity Active with a 3.3.x fork, please copy the required changes over there. It was impossible for me to review off 3.1.x since it had a merge error and included previous commits which obscured the real changes. And of course we cannot merge into 3.1.x since it does not have the changes that support the new module. Additional comments:

I continue to think a default event subscriber is not a good idea.

No problem - once commerce registration is a working example, we could put something in the README for this new submodule telling folks to look over there for guidance on how to do one, if we so desire. I assume there are tests that provide an example as well.

I dislike the button text we've ended up with.

Let me take a look once you have a new MR up in the other issue number, but I suspect I will agree and will ask you to change to "Save & confirm" or similar.

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 ...

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

It's not only for admins.

This looks really good, high quality stuff here. Ran into a couple of problems while testing:

* 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. We don't want change host to be too brittle. This exception prevents the change host page from appearing.

* That exception prevents me from even viewing or editing a registration as well. So it happens during access control when Drupal is trying to determine which tabs to show. We shouldn't do that much heavy lifting and evaluating all possible hosts in access control, access control should be lightweight, super fast and fully cached, and then when someone clicks on the change host tab they receive more information. Access control should be based on "access" aka permissions and settings, and should not be misused as "message control" in an attempt to prevent a user from ever seeing an error or warning. We want the host entity pages to be super fast and not having to recalculate access control for the change host tab every time some other host is updated.

If you can solve those two things you are pretty close. There seems to be less code than I remember from a few months ago, perhaps all that work with validation results is helping.

BTW nice work on using this same issue and updating the fork! 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, so I can learn for the next time. I closed the other faux issue I created.

Also, it should return forbidden unless the host can actually be changed (> 1 possible host), even if the user would otherwise have access. No sense in showing the Change Host tab for no reason.

I wrote ^^ 10 months ago - so it was my idea to evaluate all possible hosts in access control. My intentions were good but as I indicated a few minutes ago, 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 hundreds of pages 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.

Sorry for going back on my own request. Open to ideas but I don't think what we have now is practical.

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).

Added commit from 📌 Integrate "edit $type registration state" into access handler Active that was merged into 3.3.x

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.

pushed my new local branch

I see, do it local and push up, makes perfect sense. Thanks for the tip!

I wonder if the PossibleHost should throw an exception

There is value in showing a disabled host - the site admin who is looking to do the host change may not have realized a certain host was not configured - so I would argue for handling it softly and not an exception at any point. Avoid brittle.

The code in question is in RegistrationChangeHostManager::cloneRegistration

I'd prefer not to change type, so that field data will be preserved. Then all you have to do is update host entity ID and type. You might need a new validator in the change host module to ensure the host is the same type, and if not to give a "Different registration type" cause. Or at least make this type of check an option or setting somewhere - and if enabled, you can avoid clone/delete and update in place. It is the safer path and maybe the default when the module is installed. I am creating even more work for you - so I can add this in myself if you prefer not to tackle it. 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.

Also, go ahead with button change - "Save and confirm" works for me, since the title of that page has "Confirm" in it already, it ties things together.

Ideally the Confirm page would maintain the tabs, so the context is not lost. And have the registration # in the title "Confirm change of XXX for Registration #1". Minor nits I can handle if needed.

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.

Added commit from 📌 Determine if registrant type check can be refactored Active that was merged into 3.3.x

Merged in latest commits from 3.3.x

Feedback addressed.

I will do a little work in the next week to integrate this into my project, so might be worth waiting to commit in case I find anything unexpected.

Created follow-ups:
📌 Add a memory cache to RegistrationChangeHostManager Active
✨ Integrate waitlist with change host Active

as well as existing for commerce_registration:
✨ Allow variation to be changed after registration Active

I've integrated in my project, all looks good. It's read as far as I know.

I have a question on the one unresolved thread - once that is resolved I should be able to merge it.

Will be merging this within the next day.

I will post again tomorrow on the access issue, and also on the unpublished violation I added late but I'm having doubts about ...

Sure thing I will hold off on merging until I hear back. The access issue is resolved, at least temporarily, per my new inline comment.

(1) I've removed the unpublished violation, as I realised this wasn't right. If it was right to prevent registering for unpublished, then it should be handled upstream as part of HostEntity::isAvailableForRegistration. But actually there's good reason to allow it anyway.

(2) My commerce registration patch shouldn't exclude unpublished varations, but it should check the current user has "view label" access to the variation.

(3) I think it has to be the event subscribers responsibility to consider access when adding possible hosts.

(4) I've renamed PossibleHost::access() to ::isAvailable(), same for PossibleHostSet. This makes more sense now that we're not considering these methods in hook_access. And helps clarifies the event subcribers responsiblity about access.

(5) My concern about allowing changing host without requiring access is that this might allow an update access bypass, because we allow editing fields when changing host. A user could change host to a new host, then change host back to the old one, and get to effectively edit their existing registration even when they don't have permission to do this.

(6) A developer could remove our forbidden result by unsetting our whole hook, so it's not absolutely terminal even though it's awkward.

(7) It might be that we could do something clever with field access, like only allowing the user to edit fields on the new host that weren't present on the old host, unless they have edit access. We'd have to investigate that quite carefully, it's a bit exotic.

Should the update check be moved into the RACH then, joining the permission check? If both permissions and update should be checked, not sure why the checks should be separate.

We wouldn't have the slight problem of needing to return forbidden if we did this in RACH, that's true. But it would mean that something about registration_change_host was coded into registration core, which is inelegant if not actually problematic.

It isn't passing tests so changing to "needs work".

My concern about allowing changing host without requiring access is that this might allow an update access bypass.

I was concerned about this too. Let's leave as-is and have a separate issue to investigate options.

Fixed test fails.

Added two new issues for follow up:

📌 Allow change host access without update permission Active
✨ Allow single step change host when one registration type Active

These issues were previously created as well:

✨ Integrate waitlist with change host Active
📌 Add a memory cache to RegistrationChangeHostManager Active

john.oltman → closed merge request !36

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