As a follow-up to this issue https://www.drupal.org/project/commerce/issues/3011667 and related to this issue in reports module https://www.drupal.org/project/commerce_reports/issues/3085785 .

The reason for moving from Kerner::TERMINATE is just not to repond on the event on every single request to Drupal, because every ajax request, image thumbnail generation, REST requests and so on fire this event and it is unnecessary to respond always even when the service was not used.

Maybe I am wrong, I am opened to hear the other opinions.

The patch is attached.

CommentFileSizeAuthor
move_from_kernel_teminate_to_destructable_interface.patch3.33 KBa.dmitriiev
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

a.dmitriiev created an issue. See original summary.

a.dmitriiev’s picture

a.dmitriiev CreditAttribution: a.dmitriiev as a volunteer and at 1xINTERNET commented
Status: Active » Needs review
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro for Ny Media AS commented

The reason for moving from Kerner::TERMINATE is just not to repond on the event on every single request to Drupal, because every ajax request, image thumbnail generation, REST requests and so on fire this event and it is unnecessary to respond always even when the service was not used.

Are you sure that DestructableInterface is smarter?

From what I see, RegisterServicesForDestructionPass adds services implementing DestructableInterface to KernelDestructionSubscriber.
And KernelDestructionSubscriber subscribes to KernelEvents::TERMINATE.

So it seems like the two approaches should be equivalent performance-wise?

a.dmitriiev’s picture

a.dmitriiev CreditAttribution: a.dmitriiev as a volunteer and at 1xINTERNET commented

At least it checks if for this request the service was initialized in web/core/lib/Drupal/Core/EventSubscriber/KernelDestructionSubscriber.php:

      if ($this->container->initialized($id)) {
        $service = $this->container->get($id);
        $service->destruct();
      }

And the code is not executed e.g. on image thumbnails generation.

  • bojanz committed 2187c34 on 8.x-2.x authored by a.dmitriiev 
    Issue #3085813 by a.dmitriiev: Consider moving from Kernel::TERMINATE...
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Fixed

Okay, let's proceed. It's less code in any case :) Thanks!

Status: Fixed » Closed (fixed)

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