🇬🇧United Kingdom @jonathanshaw

Seems like the search backend from this recipe needs to be applied by default, so that other recipes providing content types can configure the search index view mode for their content type.

See setup_future_usage on the checkout pane provided.

Here are some of the pain points I've encountered with Drupal media:

(1) Image resizing. I don't want editors to have to think about image sizes. But nor do I want to send a 10mb image to a site visitor that they see as a tiny thumbnail. This seems like a basic need for almost every site out there. Configuring this to be automagically taken care of was really hard.

(2) Image cropping. Image widget crop doesn't have a great UX and is fiddly to setup. And the fact that if you reuse an image and change the crop for the new use, you also change the crop on the old use, is a disaster.

(3) I'd like to provide resources for customers at canonical pretty urls like /resources/welcome-video. I'd like to be able to add a new media like a new version of the welcome video, and make that the media shown at that canonical url. Maybe that's not so hard. But where it's gets hairy is with the URLs to the file entities. I can't stop visitors from getting the URL /resources/welcome-video/file.mp4 and sharing it, but when I add my new video it will end up as file-2.mp4. It's messy.

(4) I'd like to be able to update the titles etc of my YouTube media on Drupal and have that push an update to YouTube.

(5) I'm currently doing a lot of work on transcription and speech to text. For example, I'm preparing a transcription module that provides a transcription field to store the transcript of a media. It's a complex problem space that has only opened up recently with better STT systems and there's not a lot of prior art in the Drupal ecosystem. I'm open to contributing to a recipe here.

we will have lost the mission for Starshot itself if site builders have to search and scour to find a collection of recipes that will match the functionality they could get from installing a single, popular plugin on a competing platform.

+1

adding many recipes to one repo is jumping to a developer-focussed solution ahead of figuring out the user experience

+1

Being able to find recipes designed to work together is something to consider in #3447063: [Meta] Plan for recipe findability on Drupal.org and Project Browser →

There's a very important distinction between child->parent and parent->child we shouldn't lose track of here.

Recipes need to be able to specify parent recipes they depend on, obviously enough.

But I think the essence of the proposal here is that a parent recipe should be able to define a curated set of child recipes, of both available and suggested types.

Doing so has a significant sitebuilder impact that goes beyond discoverability: it encourages recipes to be lightweight and modular.

neither shows the duration options as a separate element. Instead, the options (and the end times that would result) are shown as options listed below the end time input when it receives focus.

I think this is an important UX win. The separate duration field is significantly confusing.

the initial display of the date and time widget is simplified, but expands to show additional inputs when it receives focus

Sounds like a good addition to the core form #states API. Accessibility would be an important consideration.

I think the fix I propose here is sound but wrong.

This permission is intrinsically confusing because of its name. Rather than fixing it to make it adhere to what we said it has been doing, but which it hasn't, lets take advantage of the fact that no one is relying on for its intended purpose, to simply kill it. Let's remove the permission, and replace it with an 'administer own host registration' permission.

Yay! @mandclu is a track lead for Starshot's events track, so hopefully we can work to make a strong integration of registration with that.

Gitlab is struggling with a end of line character it won't let me change, but otherwise all good.

jonathanshaw → created an issue.

I'm interested in this area, particularly where it bears on event registration and participation.

Registration is a very complex problem space, and I think the work done by @john.oltman in the contrib registration module is excellent. It allows registering while using any entity type as the event.

Similarly, as soon as you have events its common to want to give participants access to content partiular to that event, or give particular staff special privileges over that event. The group module is outstanding for this, but in the most straightforward scenario it requires the group entity to be the event.

My main contribution at this point is to point out that the question of what entity type the 'events' of this recipe have to be, is crucial to how useful this recipe is as a building block for more complex scenarios. It's easy enough to put together a basic events node type, but many projects will outgrow that and have to restart from scratch.

Perhaps there is a way that the recipe could focus on reusable packages like fields that could be applied to any entity type, or a way of wrapping any entity type as an event (which is what the registration module does, similar to how the typed_entity module does generically).

jonathanshaw → created an issue.

jonathanshaw → created an issue.

This has a fair number of test failures. Before spending time debugging them it would help to:
1. Have input on john.oltman on how this seems in general
2. Get 🐛 Fix access cacheability Needs review committed

So this is a simple fix, if one is willing to accept the change of behavior.

jonathanshaw → created an issue.

Here is a rough demonstration of how this might work.

If we can do 📌 HostEntity::isUserRegistered() and isEmailRegistered() don't consider edge cases Active then my question becomes:
could we make use of these new HostEntity methods in RegisterAccessCheck and RegistrationAccessControlHandler?

One possible concern is that they may be expensive, and so performance when viewing a list of hosts might degrade. I'm tempted to think we should do it anyway, rely on the render caching, and add more specific custom caching if we need to.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

I'm tempted to think this should be postponed on 📌 Add a host access handler Active and that RegistrationAccessControlHandler's logic for the 'administer' operation should include:
$result->orIf($registration->getHostEntity()->access('administer_registrations');

Widened this to include creating registrations too.

jonathanshaw → created an issue.

I opened 🐛 RegistrationAccessControlHandler checks 'administer own $type registration' incorrectly Active for discussion of the existing bug with "administer own $type registration".

The key change that's being made here is:

       -   $admin = $this->currentUser()->hasPermissions(["administer registration") || $this->currentUser()->hasPermission("administer $type registration");
       +  $admin = $registration->access('administer', $this->currentUser());

So currently only the 'administer registration' and 'administer $type registration' permissions are considered.

I think:
1. For consistency, $registration->access('administer') should check also "administer own $type registration" (which it does in the current MR, but in a buggy way). This would be a behavior change.
2. I'm not sure about "manage $type registration". Its semantics are undocumented. Adding it to the 'administer' operation would also lead to a behavior change.
3. I have wondered whether there should be an 'administer host registration' permission added here and allowing the 'administer' operation too.

jonathanshaw → created an issue.

somewhere you will need to implement the "administer" access such that it translates into checking the various administer permissions - I do not see that logic anywhere - it seems to fall through to a case where it checks the administer permissions, plus permissions that do not exist ("administer any xxx").

I think it falls through to RegistrationAccessControlHAndler::checkEntityUserPermissions(), which will end up checking the existing permissions:
administer registration
administer host registration
administer own {$entity->bundle()} registration

and the non-existent but permissions:
administer own registration
administer {$registration->bundle()} registration",
administer any registration",
administer any {$registration->bundle()} registration"

The problem is, there's a bug in RegistrationAccessControlHandler::checkEntityUserPermissions(). Currently it has:

    if ($account->id() && ($account->id() == $entity->getUserId())) {
      $own_result = AccessResult::allowedIfHasPermissions($account, [
        "administer own {$entity->bundle()} registration",
        "$operation own registration",

But in the case of the 'administer own' permission the meaning is different from that of the 'view own' etc; the own refers to the host not the registration.

As the permissions explains:

      "administer own $type_id registration" => [
        'title' => $this->t('%type_name: Administer own registrations', $type_params),
        'description' => $this->t('View, edit and delete own registrations of this type. Manage registrations and registration settings of this type for host entities to which a user has edit access.'),
      ],
      "manage own $type_id registration" => [
        'title' => $this->t('%type_name: Manage registrations for editable entities', $type_params),
        'description' => $this->t('Manage registrations of this type for host entities to which a user has edit access.'),
      ],

      "view own $type_id registration" => [
        'title' => $this->t('%type_name: View own registrations', $type_params),
      ],

view host registration:
  title: 'View host registrations'
  description: 'View all registrations for host entities to which a user has edit access, regardless of type. See this <a href="https://www.drupal.org/node/3440834">change record</a> for information about this permission.'
view own registration:
  title: 'View own registrations'
  description: 'View own registrations, regardless of type.'

I think the solution might be to use 📌 Add a host access handler Active to pass some of this work off.

jonathanshaw → created an issue.

I think this is a simple and straightforward win.

For example, if using the group module it allows group admins to override a capacity setting on a registration on a group, if a hook_ENTITY_access() implementation allows the 'administer' operation for users with a certain group permission for the group that is the registration host.

I wonder if there's also value in adding a static helper to HostEntity:

public static function create(Entity interface $entity) {
 $handler = \Drupal::entityTypeManager()->getHander($entity->getEntityTypeId(), 'registration_host_entity');
  return $hander->createHostEntity($entity);
}

This makes sense to me. There's no obvious dependency on rdf in the test, and if there's a hidden dependency someone needs to explain it.

What I wonder about is whether the explicit mark we're asserting is somehow influenced by rdf, but I don't see how it can even be installed in the test: NodeTestBase does't install it and none of the core themes tested use it.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

I don't have a specific proposal for this yet, I'm just filing this issue to flag this as a @todo.

I find the intertwined and often not DRY logic for access, HostEntityInterface::isEnabledForRegistration(), HostEntityInterface::isUserRegistered(), and RegistrationConstraintValidator rather scary. I suspect that a fundamental refactor is needed in this area. It's as if this module has done an absolutely fantastic job of growing to handle all kinds of use cases, but the growth has got to the point where it shows that one of the existing pillars needs an overhaul.

I'm actively thinking about this and hope to make a proposal.

jonathanshaw → created an issue.

I think it's possible without breaking any functionality.

I agree my concern is more with DX, trying to explain to developers working with the event what this means, why it is set or not set in this or that circumstance.

jonathanshaw → created an issue.

Advanced queue now supports unique queue items for this purpose.

I think passing $already_cloned variable would be really useful for modifying.

What do you think?

I think you might be right, but possibly this would be better done in a follow-up as it would presumably change the event for all invocations, references or not.

I don't really understand why cloneEntity() gets called on an already cloned entity in the first place.

And you guys are breaking current implementation of users which already use existing implementation of event dispatching.

I think you have to implement another type of event if you really want to have it dispatched for references.

You're right that is a risk. I think the natural pattern developers woud expect is single event, called in both circumstances. So maybe the thing to do is to create a new event and deprecate the old one, so we're not left with 2 forever.

I'd like to see what the maintainers say about this.

It turns out this doesn't actually fix my bug (it's a slippery one!). But I think it might be a good idea anyway.

jonathanshaw → created an issue.

$handler = \Drupal::entityTypeManager()->getHandler('registration', 'host_entity');
$host_entity = $handler->createHostEntity($node);
$settings = $host_entity->getSettings();

Alternatively, if your site is multilingual:

$settings = \Drupal::entityTypeManager()
  ->getStorage('registration_settings')
  ->loadSettingsForHostEntity($host_entity, $langcode);

Any reason why not this for multilingual:

$handler = \Drupal::entityTypeManager()->getHandler('registration', 'host_entity');
$host_entity = $handler->createHostEntity($node, $langcode);
$settings = $host_entity->getSettings();

How about we proceed with the unique queue support here, as it seems desirable even if not a complete solution. And do the data flag in a seperate issue?

jonathanshaw → created an issue.

Commerce has excellent support for complex multi-step checkout processes, including account registration, even without payment.

jonathanshaw → created an issue.

jonathanshaw → created an issue.

My thinking was the duplicate queue jobs would be harmless if RecurringOrderManager::createOrder() had the check I suggested.
I agree the order data seems like a natural place to store a flag.
Regarding performance, I wonder if commerce will one day change the data field to json and then we will be able to query it, something core is working on supporting ✨ Add "json" as core data type to Schema and Database API Needs work

if a job is retrying, we should not allow a duplicate to be queued.

Agreed. But the database backend sets the state of a failed job back to 'queued' if it should be retried, so duplicates will not be created. Not sure yet that we've missed anything.

Using lock for this makes me nervous, as per #88. It seems like an insufficiently robust solution, potentially with unexpected consequences or interactions with custom code, and generally increasing the WTF of an already complex mechanism.

What about #62:

I wonder if RecurringOrderManager::createOrder() should refuse to create an order if that would create two orders for the same subscription with the same billing period. Simple to enforce.

Then we wouldn't be relying on AdvancedQueue. We could still use it's mechanism to avoid duplicates, but we'd not be reliant on it.

I don't agree this is a problem.

The database backend here is being deliberately set up so that it is possible to queue the same job many times, but at any one time there can only be one of that job in the queue.

I think this is correct, because I think it's more likely the more common requirement. And anyway, it's be a serious BC break to change it now.

The simple solution is to make a custom backend for your project extending the provided one.

We could in theory provide an API for this by allowing the job plugin annotation to define which states the backend should consider for duplicates. Whether the maintainer would support this added complexity, I'm unsure.

Yes, sorry I stalled but plan to finish,!

Why remove the recursive limit?
Before the MR the limit was 20, why remove it?

Because without the MR we don't have a recursive limit, we have a repeated rendering limit. That does limit recursion, but it also blocks other valid use cases of non-recursive repeated rendering.

The fix may be wrong.

PaymentGatewayBase has this:

  protected function assertPaymentMethod(PaymentMethodInterface $payment_method = NULL) {
    if (empty($payment_method)) {
      throw new \InvalidArgumentException('The provided payment has no payment method referenced.');
    }
    if ($payment_method->isExpired()) {
      throw HardDeclineException::createForPayment($payment_method, 'The provided payment method has expired');
    }
  }

Wouldn't the correct solution be to keep the calls to assertPaymenMethod() but to override and simplify the method:

  /**
   * {@inheritdoc}
   */
  protected function assertPaymentMethod(PaymentMethodInterface $payment_method = NULL) {
    if (empty($payment_method)) {
      throw new \InvalidArgumentException('The provided payment has no payment method referenced.');
    }
  }

#161, #174, #175, #178 are beside the point I think. We know that there can be valid uses for rendering the same element multiple times in the page: that's the motivation of this issue from the beginning, and why we are building key based recursive rendering detection mechanisms. Both MR 4050 and 4994 try to prevent genuinely recursive rendering but still allow for these use cases like paragraphs, paragraphs library and simplenews.

#164/#165 is probably wrong solution as suggested in #167, they simply allow double rendering but not more. It's kind of a hack - we allow something we think shouldn't happen to happen once, but stop it after that to limit the damage. Maybe this is a clever hack, but it smells like ducking the real bug to me.

I don't like the proposal in #177 to make the recursion prevention opt-in. This is supposed to be a guard rail to help developers not shoot themselves in the foot. It should be on by default. It doesn't particularly seem right to me either to make it opt-out at the entity level. If people need to opt out, they can just use a custom EntityViewBuilder or whatever.

The immediate question is why MR 4994 is incorrectly detecting recursive rendering in webform as reported in #163, #164, and #167.

It would help if someone posted a log here: MR 4994 is supposed to log an error when it detects recursive rendering that describes why it thinks this is recursion.

#167 suggests a possible cause:

I suppose webform is really doing here something that is to be considered recursive rendering, see
https://git.drupalcode.org/project/webform/-/blob/03b6e07416480ad802f453...
This is ending up in using #theme => 'webform_submission_data' and then having this again in $variables['elements']['#theme'].

But I can't really understand what's going on there, can any one explain more?

Possibly there are similar problems with paragraphs as reported in #137 & #139. I'm not sure: has this been resolved for paragraphs in the latest version of MR 4994 or not?

One possible issue is that MR 4994 has not used revision and language keys in the way that MR 4050 did. This might explain the paragraphs problems as revisions are important in paragraphs.

More generally, the discussion in #140 / #141 about MR 4050 vs MR 4994 are still with me. I do wonder whether MR 4994 is creating fragility here and we need to move back to MR 4050. For example, does anyone have any idea whethere 4994 will work with a lazy builder? #141 raises an issue with MR 4050, but maybe there's a simpler solution to that than MR 4994.

Before we make a solution, we need to understand why the recursive rendering in paragraphs library and webform is OK. Why is it not creating an infinite regression, when our regression prevention is detecting the start of a regression?

This closely relates to #3171270: Support 'smarter saved cards' → with same proposed solution.

I'm not sure I believe point 7.

I don't see how Stripe takes payment just because a customer card is updated on Drupal.

If commerce_recurring tries to process a payment for an order for a cancelled subscription, that's an issue with commerce_recurring not commerce_stripe. If I remember right, by default commerce_recurring does exactly this, because technically the order has already fallen due and some sites might want to collect the money in this case; canncelling the sub only spares the customer from accruing future liabilities, not paying existing liabilities.

I use this code to deal with this issue in my project:

<?php

namespace Drupal\my_project\EventSubscriber;

use Drupal\commerce_order\Entity\OrderInterface;
use Drupal\commerce_recurring\Event\SubscriptionEvent;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\state_machine\Event\WorkflowTransitionEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Class RecurringSubscriptionChangeSubscriber. Provides subscriber for orders.
 */
class RecurringSubscriptionChangeSubscriber implements EventSubscriberInterface {

  /**
   * The order storage.
   *
   * @var \Drupal\Core\Entity\EntityStorageInterface
   */
  protected $orderStorage;

  /**
   * The log storage.
   *
   * @var \Drupal\commerce_log\LogStorageInterface
   */
  protected $logStorage;

  /**
   * Constructs a new PaymentEventSubscriber object.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   *
   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
   */
  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
    $this->orderStorage = $entity_type_manager->getStorage('commerce_order');
    $this->logStorage = $entity_type_manager->getStorage('commerce_log');
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events['commerce_subscription.cancel.post_transition'] = 'onCancel';
    $events['commerce_recurring.commerce_subscription.update'] = 'onUpdate';
    return $events;
  }

  /**
   * Cancel in-progress orders when a subscription is cancelled.
   *
   * OOTB commerce_recurring cancels drafts but not placed orders.
   *
   * @param \Drupal\state_machine\Event\WorkflowTransitionEvent $event
   *   The event.
   */
  public function onCancel(WorkflowTransitionEvent $event) {
    $subscription = $event->getEntity();
    $order_ids = $subscription->getOrderIds();
    if (empty($order_ids)) {
      return;
    }

    // Load outstanding orders and cancel them.
    $order_ids = $this->orderStorage->getQuery()
      ->condition('type', 'recurring')
      ->condition('order_id', $order_ids, 'IN')
      ->condition('state', 'needs_payment')
      ->accessCheck(FALSE)
      ->execute();
    $orders = $this->orderStorage->loadMultiple($order_ids);
    foreach ($orders as $order) {
      $order->getState()->applyTransitionById('cancel');
      ;
      $order->save();
      $this->logStorage->generate($order, 'my_project_order_subscription_canceled', [])->save();
    }
  }

  /**
   * Update orders when a subscription is updated.
   *
   * OOTB commerce_recurring makes limited changes to draft but
   * not placed orders. For the limited aspect see
   * https://www.drupal.org/project/commerce_recurring/issues/3200723
   * Not touching placed orders is by design; the subscription scheduled
   * changes field is designed to respect the fact that typically
   * subscribers cannot change a bill once it's fallen due.
   *
   * @param \Drupal\state_machine\Event\WorkflowTransitionEvent $event
   *   The event.
   */
  public function onUpdate(SubscriptionEvent $event) {
    $subscription = $event->getSubscription();
    $order_ids = $subscription->getOrderIds();
    if (empty($order_ids)) {
      return;
    }

    // Load outstanding orders and refresh them.
    $order_ids = $this->orderStorage->getQuery()
      ->condition('type', 'recurring')
      ->condition('order_id', $order_ids, 'IN')
      ->condition('state', ['draft', 'needs_payment'], 'IN')
      ->accessCheck(FALSE)
      ->execute();
    $orders = $this->orderStorage->loadMultiple($order_ids);
    foreach ($orders as $order) {
      $order->setRefreshState(OrderInterface::REFRESH_ON_SAVE);
      $order->save();
    }
  }

}

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

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.

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

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

Agreed.

jonathanshaw → created an issue.

@quietone I don't see any discussion in this issue about whether this is or is not a problem with php 7.4+ or php 8. Maybe it was first reported when php 7.3 was the current version of php. The test coverage provided in the patch would seem useful even if it was not a current problem.

Actually in the latest MR on ✨ Add support for unique jobs Fixed the annotation is allow_duplicates = false