- 🇩🇪Germany M_Z
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!
- Open on Drupal.org →Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7last update
6 months ago Waiting for branch to pass - 🇧🇷Brazil brunomolica
Only by adding the 'accessCheck' attribute, which is required in a Query object in D10.
- last update
6 months ago Patch Failed to Apply - Open on Drupal.org →Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7last update
6 months ago Waiting for branch to pass - First commit to issue fork.
- 🇳🇴Norway zaporylie
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.
- Status changed to Needs work
23 days ago 7:51am 16 August 2024 - 🇮🇱Israel jsacksick
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?
- 🇳🇴Norway zaporylie
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?
- 🇮🇱Israel jsacksick
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).
- 🇳🇴Norway zaporylie
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
- 🇮🇱Israel jsacksick
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().
- Status changed to Needs review
23 days ago 2:09pm 16 August 2024 - 🇮🇱Israel jsacksick
I was reviewing this one more time... And realized the following:
- 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?
- It could be that we get the same promotions from both methods, causing the same promotion to be loaded / saved multiple times?
- There is no limit to the query, perhaps it'd be safer to limit the number of results returned (100 for example?)
- 🇮🇱Israel jsacksick
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. - 🇮🇱Israel jsacksick
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...
- 🇳🇴Norway zaporylie
Chatted with Jonathan on Slack and decided to keep the logic as is for readability but introduced a query limit for safe measure.
-
jsacksick →
committed 4f353972 on 8.x-2.x authored by
zaporylie →
Issue #2863864 by zaporylie, brunomolica, igorbarato, jsacksick: Disable...
-
jsacksick →
committed 4f353972 on 8.x-2.x authored by
zaporylie →
- Status changed to Fixed
19 days ago 12:41pm 20 August 2024 -
jsacksick →
committed 3a06ae22 on 3.0.x authored by
zaporylie →
Issue #2863864 by zaporylie, brunomolica, igorbarato, jsacksick: Disable...
-
jsacksick →
committed 3a06ae22 on 3.0.x authored by
zaporylie →
-
jsacksick →
committed 0c8640ae on 3.0.x
Issue #2863864 followup: Remove the deprecated CronInterface.
-
jsacksick →
committed 0c8640ae on 3.0.x
Automatically closed - issue fixed for 2 weeks with no activity.