🇬🇧United Kingdom @jonathanshaw

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 RTBC the annotation is allow_duplicates = false

Although I've not been active on this module for a while, there's also not been much other development. Therefore I've gone ahead and made ptmkenny a maintainer as I know from working with him elsewhere that he is very knowledgeable and conscientious in this role.

Hi @joe, I'm not sure. The billing profile is specific to the order; but the Drupal user is counterpart to stripe customer, the stripe customer id is stored on Drupal user entity I think. If the billing profile has the better info in a certain use case, maybe the display name needs to be updated to use it.

I got a fatal error - Drupal\advancedqueue\Exception\DuplicateJobException: Job rejected because it duplicates existing jobs.

I guess, we should simply not add the second job to the queue and add a log with a notice.

Yes, that behavior is by design. This strategy was chosen by the maintainer in #114:

As an API user, I'd try to create the job, and if I get the exception, I either propagate the error up, or I do something to make the job unique and try again. Normally, the API user is the one that would better understand the consequences of trying to create duplicate jobs.

I've needed this too, for canAddNew().

Looks good, RTBC when phpcs is fixed.

Created a CR

I think this is ready again.

@dww you may be encountering #35 / #47

And the tests fail all over the place and weirdly ... some debugging needed.

I've made an attempt, let's see what the tests say.

It turns out that this bit of SpoolStorage::addIssue():

// Save except if already saving.
 if (!isset($issue->original)) {
    $issue->save();
 }

is a problem. It's actually already a problem. It's designed to prevent an unnecessary recursive save when addIssue() is called from simplenews_node_presave(). But ->original is still set during insert and update hook invocations, so it also currently stops the updated issue stats and status from being set properly when addIssue() is called from an update hook. Which doesn't matter for nodes currently, but messes with anyone using simplenews with another issue entity type and sensibly using insert/update hooks instead of presave.

Unfortunately, I cannot see a sensible way to detect whether addIssue() is called from a presave hook or an insert/update hook. We can in theory play around with setting a flag presave and removing it postsave, but it's more or less impossible to guarantee this works given the places stuff can be called from like hook_ENTITY_TYPE_presave, hook_entity_presave and just EntityBase::preSave(); even trying to get this to work imperfectly would involve fiddly stuff with hook_module_implements_alter().

I think the best solution is to accept that non-node entities are getting buggy results right now, that even if they are calling addIssue from a presave hook they will probably survive us triggering a recursive save OK, so we'd best just fix this right as there's nothing else we can do to help them. Unless we're willing to deprecate addIssue() altogether as beyond BC redemption.

Thanks for the reply Adam. I was about to open an issue with scheduler, but then I realised what their maintainer would probably say:
why is simplenews taking a heavy action like spooling an email in a presave hook.

Surely presave hooks are for modifying the state of an entity before it is saved, for taking a downstream action consequent on an entity being saved in a particular state we should wait for the entity to actually be finished saving, and use an insert/update hook instead.

Potentially this could also improve the maintainability of SpoolStorage::addIssue() which is called from simplenews_node_presave(). It does things like:

// Save except if already saving.
    if (!isset($issue->original)) {
      $issue->save();
    }

which seems a bit twisty.

Makes sense. Woo hoo for this issue being landed!

Improved release snippet in IS

See #5. In some circumstances the webform parameter is not upcast by the route.

Added steps to reproduce

Thanks @adamfranco, I fixed the variable name in the update hook.

Implemented double underscores.

Although

$hooks[] = "plugin_filter_{$type}";
$hooks[] = "plugin_filter_{$type}__{$consumer}";

does uses only 1 double underscore, I agree with @alexpott that we should do

$hooks[] = 'entity_query_tag__' . $tag;
$hooks[] = 'entity_query_tag__' . $this->getEntityTypeId() . '__' . $tag;

The reason I can see is that 'tag_something' is a possible entity type id.

It's annoying for simplenews to have to do this, but I don't see an alternative solution and scheduler has 65k D8+ installs. Scheduling an email newsletter to go out at a future time would seem to be a common use case.

Thanks @manish-31. Tests are now green. This is still NR for the changes made in response to #70, but otherwise RTBC per #68.

Green at last.

You have to trigger cron often enough to to be able to execute the most frequent cron job.

We should document this.
The D7 version talks about needing to configure cron to run every minute, which made me wonder if this was required for D8+.

Re #50 \Drupal\Core\Executable\ExecutableInterface is very much part of the plugin API, whereas tagged services are used here.

I understand the concerns with runCron(), it suggest initiating a whole global cron in the way the \Drupal::service('cron')-run() does now. But I also understand Berdir's concern that onCron() still suggests a single global cron event, which we are moving away from.
I think there's merit in talking of a "xxxCronJob" as @dww suggests or "xxxCronTask".
But how about just execute()?

Unfortunately entity query classes do not inherit their execute() method from the abstract class QueryBase, so there's no way to guarantee all entity queries have this.

Added support for config entity queries and test coverage for that, documented in CR and entity.api.php the limitations on which entity queries have these.

Thanks @berdir for thinking of config entities.

This issue is mentioned in Media::prepareSave():

  public function prepareSave() {
    // @todo If the source plugin talks to a remote API (e.g. oEmbed), this code
    // might be performing a fair number of HTTP requests. This is dangerously
    // brittle and should probably be handled by a queue, to avoid doing HTTP
    // operations during entity save. See
    // https://www.drupal.org/project/drupal/issues/2976875 for more.

Awesome working with you @ptmkenny :)

I beleive youtube returns 303 response codes for short urls like the one you give, but you can configure linkchecker to ignore 303 response codes.

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.

You're right. This validator would be very hard to use sensibly on a non-user entity, however we tweaked it, and we don't do anyone a favour by allowing it.

I like that you used an exception here.

I've added a few nits on the MR, feel free to ignore.

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

You're right to remove the second param.

The test seems to be is looking at what happens when there are 3 users, 2 caught by one condition, 2 caught by an or group, but only 1 caught by both.

I suspect there's no great logic to which things get tested here - the query altering is pretty hairy stuff, so I probably justed a whole bunch of ways of querying stuff in a shotgun approach and hoped I tested what I needed to.

I'm happy with that. A pit puzzled by where this static is, I can't see one :)

This message: 'The alternative email address %value is already associated with another user account.' is no longer quite right if this validator is being used on the core 'mail' field as well.

Sorry, you're obviously right! I just somehow missed that was in gitlab CI and assumed it was in composer.json. Thanks for educating me:)

I like where this has got too. It seems clean and simple, but flexible enough to accomodate any use case.

Thanks to everyone for the feedback.

What I've done is:
- simplified the job type annotation to a boolean
- given the job type plugin responsibility for deciding how duplicates should be handled
- made the base job type plugin's implementation simply throw an exception if there's a duplicate
- given the Queue entity, not the Database backend, responsibility for looking for duplicates.

This means that all of the duplicates functionality added in this issue is completely bypassed if you call $queue->getBackend()->enqueue(). But if you call $queue->enqueue() you get safe behavior whatever backend you use.

I believe in a follow-up issue we should deprecate Queue::getBackend(). This function creates a leaky abstraction of sorts; by putting API users in direct contact with the backend, we lose any ability to intermediate. Instead API users should interact with the queue entity, and we can use that to provide safety and backwards compatability as we add new features like this in future.

It's a good solution.

However AltenativeUserEmailsValidator does this:

if ($entity->getEntityTypeId() !== 'user' || $items->getFieldDefinition()->getName() !== 'alternative_user_emails') {
return;
}

so the tests shouldn't be passing, which is a little concerning.

I don't see why we can't just remove that whole early return block. What's the harm if people want to use this validator somewhere else, that's up to them.

I'm using the merge request result directly, which I know can be a little dangerous, but since this module is dependent on that patch, I would rather have the tests start failing if the MR is changed in an incompatible way so that we can be aware of it.

I think that's right from the point of our tests. But I'm concerned that this patch bubbles up into actual projects that use this module, which could break projects. The MR is against D11, and the latest version of the MR might stop applying at any time on on a D9 or D10 project.

Unfortunately I can't see a way to specify that a patch is a dev dependency not a production one.

I can't see we have an alternative but to include a downloaded patch with the module.

Alas this all seems pretty horrible to me from a release management point of view - as we need a different patch for D9 and D10 we should in theory run a different branch for each, identical except for the different patch file. However, I don't mind just dropping D9 support altogether and hope that the same patch keeps working for D10 and D11 for now.

What do you reckon?

I've opened a new MR as I'm making a significant number of changes here:
- using 'duplicates' as the key term instead of 'unique'
- introducing the term 'duplicates policy' to describe the duplicates handling strategy specified on a job type.
- using 'fingerprint' instead of 'unique id' or 'job hash'
- removing jobTypeRequiresUniqueness() and getJobTypeUniquenessType() methods from the JobTypeManager, in favour of new method Job::getDuplicatesPolicy()
- removing SupportsUniqueJobsInterface altogether, adding no new interface
- adding a verifyJobSupport() method to BackendInterface and BackendBase.

However, doing this I've realised that a lot of the reason why this issues is getting tricky goes back to #20:

Backends may or may not support unique jobs.

However, silently ignoring jobs that should be unique is dangerous, ... Commerce Recurring's jobs absolutely MUST NOT be used with a backend that does not support unique jobs, and therefore, I think we should guard against this here.

So therefore, I want to introduce a concept of a job type *requiring* that its jobs be treated as unique. This can be declared on the job type plugin annotation.

This goal makes sense. However, it is not really acheivable for the reasons pointed to in #93. It is perfectly possible and reasonable for custom code using this module to enqueue jobs with:
$queue->getBackend()->enqueueJobs([$jobs]);
This means that calling code has direct access to the backend plugins. We can't stop calling code from passing jobs to backend plugins that can't properly handle them, and we can't force custom or contrib backend plugins to verify they can handle a job properly.

Everything we are doing with SupportsUniqueJobsInterface, Queue::verifyQueueSatisfiesJob(), BackendInterface::verifyJobSupport(), JobTypeManager::jobTypeRequiresUniqueness() etc is basically wishful thinking. We are trying to create protective mechanisms but we can't ensure that anyone uses them.

I think at this point we should:
- rip out these mechanisms (which in MR11 are basically simplified to BackendInterface::verifyJobSupport())
- create a new issue for discussing and working on this rather tricky problem, which may apply to other future features not just this duplicates feature
- document on the annotation type that duplicates handling may not be safe when using backend other than the Database backend.

I agree that BackendBase::ensureUniqueJobsSupport() does not seem necessary. It's unused in the Database backend which is the only one we offer.

I agree SupportsUniqueJobsInterface doesn't need a public getDuplicateJobId() method.

I dislike the jobTypeRequiresUniqueness() and getJobTypeUniquenessType() methods on the JobTypeManager. I understand the logic for wanting to check this without instantiating a plugin, but the semantics seems very ugly. I think it would be nicer to have this as just a single method on the Job class: Job::getDuplicatesPolicy() which itself calls out to JobTypeManager::getDefinition().

I agree $this->getBackend()->supportsJobType($job_type); is a nicer semantic than trying to figure this out on the Queue entity or in the JobTypeManager. Actually if we do this, then strictly speaking we don't even need the SupportsUnique/Duplicates interface at all.

I think part of the problem we're having with naming things and figuring out how this should be organised is that the semantics of 'unique' are unhelpful.

Let's discard the whole term "unique" and focus on the language of duplicates and how we handle duplicates: we need to detect duplicates (by hash), and then manage them.

Concretely this leads me to the following suggestions:
instead of a single Drupal\advancedqueue\Plugin\AdvancedQueue\Backend\SupportsUniqueJobsInterface we have:
Drupal\advancedqueue\Plugin\AdvancedQueue\Backend\SupportsDetectingDuplicateJobsInterface

Our annotations become:

/**
   * Allow duplicate jobs works with any backend.
   */
  public const DUPLICATES_ALLOW = 'allow';

  /**
   * Do not enqueue duplicate jobs. Requires a backend implementing SupportsDetectingDuplicateJobsInterface.
   */
  public const DUPLICATES_SKIP = 'skip';

  /**
   * Merge with existing job. Requires a backend implementing SupportsDetectingDuplicateJobsInterface and SupportsDeletingJobsInterface.
   */
  public const DUPLICATES_MERGE = 'merge';

Notice that DUPLICATES_MERGE I've said requires SupportsDeletingJobsInterface instead of some new SupportsUpdatingJobsInterface - this is because the simplest possible way of updating a job is to delete the old one and insert a new one.

I'm saying 'merge' here instead of the previous 'overwrite' because the basic idea in order to avoid committing us to what the payload should be of the final updated job; in this first implementation we should just overwrite the old job with the new one, but in future we might allow the job type plugin to have an opinion about how the payload of old and new job are combined.

You're right, we've got smacked by 📌 UniqueFieldValueValidator works only with single value fields Fixed .

So perhaps the best approach is to override UserMailUnique with a custom validator?

That sounds like a sane and maintainable solution. It was kind of neat before that just altering the entity query took care of validation magically, but a custom validator is fine.

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.

I agree, but I think we should get tests passing before we merge this.

This is copied from core's UniqueFieldValueValidator.

But PhpStorm is right, we never use the variable. So we should do $items->isEmpty() instead.

This needs to add the core patch using composer patches.

I am not actively using it, and not sure I will.

jonathanshaw → created an issue.

jonathanshaw → made their first commit to this issue’s fork.

Use case: as a site administrator, I can install a new site using existing configuration so that I can for example clone new development environments

Solved in core.

Use case: As a site administrator I can review and safely import available configuration updates from installed modules, themes, and the install profile without overriding customizations I've made on the site

Solved by widely used contrib Configuration Update Manager

Use case: As a module developer or site administrator, I can produce a package of configuration that meets a distinct use case

Targeted by Recipes initiative.

I agree with @catch. This issue is a proposal for an initiative. It has not gained traction, and is now outdated. It clutters the issue queue and to focus attention better we should close it as outdated.

Many issues still exist in this area, but as an initiative proposal this should be closed. If there's a new initiative other than recipes that is needed, it would do better to start from scratch as a new more focused proposal.

If there's

I think I'm wrong above in #55.
I don't see a way to get information back from the cacheable metadata added by hooks about what createAccess() $context they used in their logic. Therefore we cannot handle context intelligently even with MemoryCache.
So we either have to serialize the whole context, which has performance concerns, or we just don't do static caching when there is context. Which is the approach in #36 we mostly all like.

This is valid regardless of whether we use MemoryCache or not in the future. $context could contain anything, and so we shouldn't automatically be stuffing it into MemoryCache either.

MemoryCache would solve the issue identified in #48, but it's not the right solution for this, so let's rule #48 out of scope here and move it to another issue.

We had converged on consensus on the solution in #36. But #48 makes this point:

including the $context that's in createAccess() isn't a complete fix for this bug: A module might implement hook_entity_create_access() and return an access value that's based on something else. It would correctly add a cache tag for that dependency into the returned AccessResult, only to have that ignored by the static caching.

I think this is a valid concern.

It seems therefore that in order to fix the bug here, we should use MemoryCache.

Re #20 from memory:
1. Sitebuilder: Nest once complex IEF inside another
2. Sitebuilder: Set the inner one to auto open
3. User: Open an entity in the outer IEF; SUCCESS the inner IEF auto opens properly.
4. User: Cancel the opened outer IEF
5. User Open the same entity again in the outer IEF; FAILURE the inner IEF does not auto open now!

Unfortunately the approach here is blocked by an upstream issue ✨ Add PRE_CLONE/POST_CLONE event dispatch handling to sub referenced entities Needs work . So we should postpone this on that.

jonathanshaw → changed the visibility of the branch 3058025-add-preclonepostclone-event to hidden.

I believe the approach here is better than #3089105: When cloneReferencedEntities is called it should dispatch events → therefore I have closed that one as a duplicate.

What's better about the approach here is that is moves the events into the cloneEntity method, whereas the other approach keeps the events in the form, but also calls them when cloning child entities. The approach here avoids complicating the code base, and keeps in the direction of seperating the API from the UI.

I've moved the test across from #3089105: When cloneReferencedEntities is called it should dispatch events → and suggest maintainers credit contributors to that issue.

I think this MR may have caused tests to fail previously when it was committed becaue it was buggy: in the committed version the pre-clone event dispatching was both left in the form and moved into the handler, so the dispatch was duplicated.

Closing as a duplicate of ✨ Add PRE_CLONE/POST_CLONE event dispatch handling to sub referenced entities Needs work and moving the test to there. Will ask maintainer to transfer credit to.

jonathanshaw → changed the visibility of the branch 3058025-2 to hidden.

jonathanshaw → made their first commit to this issue’s fork.

This is not caused by this patch or fixable here. Likely it's because the real name is not properly available for a new user.

You might be able to workaround that using the display name alter hook.

You could also check for and/or create an issue for this module to add an event to allow customising the Stripe customer metadata.

Thanks @smustgrave, that sorted it.

The spellchecking CI seems to be broken ... but I think this is right.

I'd like is to to decide the hook name structure before I change the code as it will need changing in quite a few places.

I don't have a strong sense either way about the order.

Also if a tag and entity type ID clash your site will have problems and that means this needs work.

Good point. I'm a bit unsure about this one though:

$hooks[] = 'entity_query_tag_' . $this->getEntityTypeId() . '_' . $tag;

Strictly _tag_ is not needed here to avoid collision but I can see that it might be more consistent to have it.

I wondered if this is better:
$hooks[] = 'entity_query_' . $this->getEntityTypeId() . '_tag_' . $tag;

jonathanshaw → made their first commit to this issue’s fork.

Looking at how SQL queries are altered - it only triggers the alter hook if the query is tagged. See \Drupal\Core\Database\Query\Select::preExecute(). I'm wondering if we should do the same.

I'm not sure why we would do that. Mind you, I'm not sure why SQL queries do that. Any ideas why it would be good to do this?

Being able to alter absolutely any entity query would seem like a useful feature for modules like DER or workspace that want to globally enhance entity queries.

some tags that you'd expect to be there like ENTITY_TYPE_ID_access are only added in \Drupal\Core\Entity\Query\Sql\Query::prepare() which is called after this.

I think that might be good. In some sense these access tags seem like part of a deeper level of the API. If you want to mess with an entity query's access should do do it through accessCheck() which is it's API for the purpose; or you accept the greater responsibility of altering the Sql query.

Unfortunately in the current design it does not seem possible for a hook implementation to know if \Drupal\Core\Entity\Query\QueryBase::$accessCheck is TRUE or not.

Agreed. This is one symptom of the need identified in #2502363: Add getters to EntityQuery → . Once this issue is fixed, it will get more valuable to fix that one.

Also I'm wondering about the impact on the existing entity_query and entity_query_ENTITY_TYPE_ID tags. They feel somewhat superfluous and duplicates.

Maybe - although they allow a deeper level of sql modification than entity queries currently do. Shall I open a follow-up to explore deprecating them?

Another thought is in what order should the hooks be called. Obviously hook_entity_query_alter() first... but then I think it should be hook_entity_query_TAG_alter() then hook_entity_query_ENTITY_TYPE_alter() and then hook_entity_query_ENTITY_TYPE_TAG_alter()

Entity/Query/Sql/Query::prepare does this:

    $this->sqlQuery->addTag('entity_query');
    $this->sqlQuery->addTag('entity_query_' . $this->entityTypeId);

    // Add further tags added.
    if (isset($this->alterTags)) {
      foreach ($this->alterTags as $tag => $value) {
        $this->sqlQuery->addTag($tag);
      }
    }

Which means that sql queries alter hooks fire in the order ENTITY_TYPE then TAG (as is the current MR) not the TAG then ENTITY_TYPE you propose. I suggest we keep to the same pattern as sql queries unless we have a good reason to be different.

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.

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.

Sorry for the noise, I didn't seem Victor had just opened 💬 Support Entity Clone Active for me.

Reopening as entity_clone support is still relevant. Even if maintainer explicitly does not wish to maintain code for this, documentation or community discussion of solutions seem valid. I hope to create an MR soon with code.

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.

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.