Disable promotions via cron if end date or usage limit hit

I'll be getting at this during the week.

Pull request - https://github.com/drupalcommerce/commerce/pull/708

Matt, I pushed changes addressing your suggestions, ready for another look.

PR updated, rebased since it's been a little while since this was last looked at.

I catch the patch placed on GitHub and updated it.

Fixed loadExpired() to get right timezone.

/modules/promotion/src/PromotionStorage.php (Line 166)
-      ->condition('end_date', gmdate('Y-m-d'), '<')
+      ->condition('end_date', date('Y-m-d\TH:i:s'), '<')

Replaced undefined function to load multiple usage promotions.

/modules/promotion/src/PromotionStorage.php (Line 194)
- $promotion_uses = $this->usage->getUsageMultiple($promotions);
+ $promotion_uses = $this->usage->loadMultiple($promotions);

Fixed PHPLint error.

I tested the patch #10 for Drupal 9.5.9 and Commerce 8.x-2.31 and it works perfectly.

A promotion that has reached the maximum usage and gets deactivated after a cron run.

Don't forget to clear cache after patching the commerce_promotion sub-module.

+1 RTBC

Some should test the patch for Drupal 10 and latest Commerce version!

Only by adding the 'accessCheck' attribute, which is required in a Query object in D10.

Update a new file because previous doesn't work

It is the last update from this version.

I took #14 and turned it into MR. I had to fix tests because they were broken. I also moved cron into its own service class.

I do feel like this feature is needed but at the same time, I am not convinced we need to expand the promotion storage. I see a limited need for loading maxed and expired promotions outside this functionality therefore I would rather keep queries as part of the cron service for now and perhaps promote them to the storage if we find a use-case for that.

I agree, not sure there is a need for these 2 new methods on the promotion storage, might make more sene to just add helper protected methods directly in the Cron service.

Also, several other comments:

• There is no need for a CronInterface, Drupal core already defines one
• I think the loadMaxedUsage() logic should be rewritten to perform direct DB queries with joins... We can easily join on the promotion usage table. The current logic is inefficient... We can directly query the promotions that need to be filtered out.
• There is no way to opt out for this behavior, wondering if we shouldn't add settings for that... For example, a merchant could decide to extend the promotion dates, realizing too late that it had expired for example?

There is no need for a CronInterface, Drupal core already defines one

Good catch. I noticed commerce_cart also defines its interface so I just followed that strategy. But I agree that better to use core's croninterface if possible.

I think the loadMaxedUsage() logic should be rewritten to perform direct DB queries with joins... We can easily join on the promotion usage table. The current logic is inefficient... We can directly query the promotions that need to be filtered out.

I believe the problem here is that usage was defined in #2763705: Implement a usage API as backend_overridable so we cannot rely on it being in that db table.

There is no way to opt out for this behavior, wondering if we shouldn't add settings for that... For example, a merchant could decide to extend the promotion dates, realizing too late that it had expired for example?

Not sure if that's a big of an issue? One can still extend the promotion and activate it. And if the promotion end date passes while the promotion is still active (as it works currently) the promotion is not applicable anyway. Is there any other factor that should be considered here?

I believe the problem here is that usage was defined in #2763705: Implement a usage API as backend_overridable so we cannot rely on it being in that db table.

Indeed good point, we need to use the services / API, that may result in poor performance but I guess we can't really workaround that.

Also, could you please use property promotion (for the Cron constructor).

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

It turns out that core's CronInterface is meant less for a single cron task handler and more for the runner as it expects the return type. Do you still believe it's a good fit? I noticed it only because of failing phpstan test in https://git.drupalcode.org/issue/commerce-2863864/-/jobs/2460087

As discussed on Slack, let's define a CronInterface from Commerce core, with a void return type.
Also, let's try to tweak the comments a little bit...

Returns active promotions which have a met their maximum usage.

perhaps should be

Returns active promotions which have met their maximum usage.

?

Also,
$promotion_uses = $this->promotionUsage->loadMultiple($promotions);

maybe just:
$usage = $this->promotionUsage->loadMultiple($promotions);

Let's also refactor:

    foreach ($promotions as $promotion) {
      if ($promotion_uses[$promotion->id()] >= $promotion->getUsageLimit()) {
        $maxed_promotions[] = $promotion;
      }
    }

This to use array_filter().

Addressed comments in #22

I was reviewing this one more time... And realized the following:

1. We could potentially perform a single query? And add an OR condition group, get either promotions with an end date in the past or with an usage limit? That may make the code less readable, but it's more performant?
2. It could be that we get the same promotions from both methods, causing the same promotion to be loaded / saved multiple times?
3. There is no limit to the query, perhaps it'd be safer to limit the number of results returned (100 for example?)

Maybe we just introduce a getPromotionsToDisable() method that performs a single query?
From there we'd then need an array_filter() or a loop that returns the promotions that have an end date in the past or that is maxed used.

The one drawback of a consolidated query is that makes the code less readable and that forces the PHP code to implement "duplicated" logic to check if the promotion expired...

Chatted with Jonathan on Slack and decided to keep the logic as is for readability but introduced a query limit for safe measure.

Merged, thanks!!