Support priority for needs_destruction service tag

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Moving back to NW for issue summary update, change record, and test coverage

#25 signed of on not needing additional tests, the existing coverage should be enough

Updated the issue summary a bit to match the proposed fix.

Will ping a release manager about #25 on the remove vs deprecate. That also influences the CR so not adding that until we know how we want ot use it

Per @catch on slack:

I also consider it internal but I think we could do a bare minimum deprecation (no test coverage, just the @deprecated stuff) just in case someone is doing something weird somewhere.

Re-roll for Drupal 10.

Converted to MR.

Still NW per #38

What is being deprecated?
Maybe I am missing something..

We should leave RegisterServicesForDestructionPass in place and tag it as @deprecated in 10.3.x for removal in 11.0.x.

bradjones1 → closed merge request !6107

bradjones1 → changed the visibility of the branch 3038040-support-priority-for to hidden.

Refactored this to use a service collector. Per #38 deprecated the subscriber however we'll still call any services that are added in any overridden implementations of the current subscriber. I think this is the right way to do this?

Do we need tests for this? I think maybe not, since we'd just be duplicating testing of the tag collector...?

The test failures are... strange... but it's late. Will circle back tomorrow.

Merged in the latest 11.x branch. The same single test is still failing. These queries are not expected in core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php

+    34 => 'INSERT INTO "semaphore" ("name", "value", "expire") VALUES ("active-trail:route:view.frontpage.page_1:route_parameters:a:2:{s:10:"display_id";s:6:"page_1";s:7:"view_id";s:9:"frontpage";}:Drupal\Core\Cache\CacheCollector", "LOCK_ID", "EXPIRE")',
+    35 => 'DELETE FROM "semaphore"  WHERE ("name" = "active-trail:route:view.frontpage.page_1:route_parameters:a:2:{s:10:"display_id";s:6:"page_1";s:7:"view_id";s:9:"frontpage";}:Drupal\Core\Cache\CacheCollector") AND ("value" = "LOCK_ID")',

The menu.active_trail service is marked with needs_destruction, so it makes some sense / appears to have some relation, but not that much. What appears to have changed is that the active trail service comes into use at all during this test. And I don't understand why this change to destructable services would change whether the active trail service is used - one would think this would have been happening before, and therefore the test failing before.

I'm not able to apply a diff or patch for MR8127 to 10.3.x core, and #39 doesn't seem to apply on 10.3 anymore either. Here's a version of the current MR8127 made against 10.3.x specifically.

Rebasing is preferential IMO to merging in from upstream, it helps keep the changes in the MR clearer. But, whatever, certainly not a hill to die on.

I think the performance test query list is more about order of operations and may even reveal a lack of test coverage - I'm running into that a lot, lately. Not sure it's really a failure vs. an opportunity to update the test.

Reroll for latest 10.3.x

Ignore my previous comment with patch, #54 should work well (I just had a conflict with another issue)

Wouldn't be more easy to use the Symfony's PriorityTaggedServiceTrait::findAndSortTaggedServices() and just update RegisterServicesForDestructionPass instead of introducing a new service?

Also, this needs some coverage.

I've added an alternative approach based on #59 (let's see the bot)

The approach from MR !9853 still needs tests. .Apart from that I see a consistent failure with StandardPerformanceTest which I could not relate to this change.

MR!9853 looks like an interesting approach as it changes much less code. It has uncovered the same issue as noted in #53 where there are two extra database queries; it would be good to understand why this happens.

The easiest way to add test coverage for destruction ordering will be to extend ServiceDestructionTest I think.

It has uncovered the same issue as noted in #53 where there are two extra database queries; it would be good to understand why this happens.

I smell here an unrelated bug. Not long ago I have noticed a weird behavior with destructable services. As I remember it was a service not available anymore

Here's my analysis

How I've tested?

I've applied this patch to DrupalKernel to record the sequence of "needs destruction" services:

--- a/core/lib/Drupal/Core/DrupalKernel.php
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -687,9 +687,13 @@ public function terminate(Request $request, Response $response): void {
       // service was not used.
       foreach ($this->container->getParameter('kernel.destructable_services') as $id) {
         if ($this->container->initialized($id)) {
+          file_put_contents('/tmp/dump.txt', "$id\n", FILE_APPEND);
           $service = $this->container->get($id);
           $service->destruct();
         }
+        else {
+          file_put_contents('/tmp/dump.txt', "$id --not-called\n", FILE_APPEND);
+        }
       }
     }
   }

Results

11.x HEAD

state
module_handler
theme.registry
library.discovery
block_content.uuid_lookup --not-called
path_alias.whitelist --not-called
path_alias.prefix_list
Drupal\performance_test\PerformanceDataCollector
drupal.proxy_original_service.menu.active_trail --not-called
drupal.proxy_original_service.router.builder --not-called

MR !9853

state
module_handler
theme.registry
library.discovery
block_content.uuid_lookup --not-called
path_alias.whitelist --not-called
path_alias.prefix_list
drupal.proxy_original_service.menu.active_trail
drupal.proxy_original_service.router.builder --not-called
Drupal\performance_test\PerformanceDataCollector

It's easy to observe the issue. In 11.x HEAD, Drupal\performance_test\PerformanceDataCollector service runs before drupal.proxy_original_service.menu.active_trail, so it fails to collect queries ran by the later. Weird here (or maybe not?) is that drupal.proxy_original_service.menu.active_trail is not called at all because is not initialized (see the code from DrupalKernel).

With MR !9853, the priority of PerformanceDataCollector (which is -1000) is honoured, so the collector allows all other services to run first and, voilà!, we have 2 additional queries which are related to active trail.

I'm changing this to be a Bug because we see services relying on priority (the performance collector) but they fail to run at the end, as expected.

But a question remains: Why drupal.proxy_original_service.menu.active_trail was not called at all with 11.x HEAD?

Added missed queries. Also, because menu.active_trail destruction failed to run the cache set/get counts were also not accurate.

This is not ready because it still lack test coverage. However, I'm setting to "Needs review" to get some feedback on analysis but also on the question from #67 ✨ Support priority for needs_destruction service tag Needs work

claudiu.cristea → closed merge request !8127

• Test coverage was adde (removed tag).
• Pipeline is green. I've also ran "test only" job to prove the bug.
• Closed the other MR

Ready for review.

Did a global search for projects defining destructable services with priority https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22needs... . It turns out that performance_test module is the only one doing it. I think it's correct to fill this as a bug.

Hide patches.

Updated IS

Rebased after 📌 OOP hooks using attributes and event dispatcher Needs review

Probably we can use PriorityTaggedServiceTrait::findAndSortTaggedServices() to simplify code in other DI classes. But that could be for a followup

I didn't know about PriorityTaggedServiceTrait prior to this, but like how it is purpose-built for this. We just need to implement it. Very straightforward, I think this is RTBC.

Committed/pushed to 11.x, thanks!

This doesn't cherry-pick cleanly to 10.4.x. I think it is probably valid for that version, but also not the end of the world if we don't backport it. So marking fixed, but happy to backport if someone opens a backport MR.

catch → closed merge request !9853

Added a new 10.4 MR

catch → closed merge request !9916

Thanks for the quick turnaround on the backport.

Committed/pushed to 10.4.x, thanks!

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