Kernel::TERMINATE event subscriber that is used right now for generating the reports is probably good for frontend performance, but is fired every time there is a request to Drupal, it means all ajax requests, image thumbnail generation request and so on.

Would not it be better to use DestructableInterface for the service instead? I think it has the same advantages as Kernel::TERMINATE subscriber, but is called only when the service was actually called.

Maybe I am wrong, it would be nice to here another opinion.

Attaching the patch here for now. Please review.

CommentFileSizeAuthor
#4 3085785-4.patch6.05 KBjsacksick
destructable_interface.patch6.67 KBa.dmitriiev

Comments

a.dmitriiev created an issue. See original summary.

a.dmitriiev’s picture

a.dmitriiev commented
Status: Active » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/src/OrderReportGenerator.php
@@ -96,4 +108,24 @@ class OrderReportGenerator implements OrderReportGeneratorInterface {
+  public function flagOrder($order_id) {
+    $existing = $this->state->get('commerce_order_reports', []);
+    $existing[] = $order_id;
+    $this->state->set('commerce_order_reports', $existing);

maybe better to use queue or at least lock service to prevent race

jsacksick’s picture

jsacksick commented
StatusFileSize
new 3085785-4.patch6.05 KB

I don't see a reason to use the state, attaching patch is getting rid of that, let's see if the tests are still passing.

  • jsacksick committed 9481750 on 8.x-1.x 
    Issue #3085785 by a.dmitriiev, jsacksick, andypost: Generate order...
jsacksick’s picture

jsacksick commented
Status: Needs review » Fixed

Went ahead and committed the patch from #4.

Status: Fixed » Closed (fixed)

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