- Issue created by @berdir
- Merge request !10798improve isPendingRevisionSupportEnabled implementation β (Open) created by berdir
- π¨πSwitzerland berdir Switzerland
Unsure what to do about the cache tags. We can depend on entity entity_bundles cache tag, because they get invalidated on any workflow change. But that implies that we know about the implementation of the content moderation API that we're using.
- π¨πSwitzerland berdir Switzerland
Ok, this has a problem because it already needs this information in hook_entity_bundle_info_alter(). And we can't run after content_moderation, because the alter hook in content_moderation in turn depends on the translatable flag to be set.
Added the direct check using workflow config entities for this case.
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Replace with a workflow_list cache tag. That said, this alone won't help, since the implementation then again loads all workflow entities.
Why would adding a list cache tag load all entities?
In case you're worried there are too many workflows that are not related to content moderation, we could introduce a few custom list cache tags in workflows so you can have something like: workflow_list:type:content_moderation
- π¨πSwitzerland berdir Switzerland
It used to, but the new implementation no longer needs to load.
- πΊπΈUnited States smustgrave
Is there any performance measurement that may help with the review?
- π¨πSwitzerland berdir Switzerland
This (authenticated user + content moderation + content translation) is currently not a scenario that has explicit enough test coverage to cover this. The test methods in \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest hit this scenario and there are issues to expand explicit assertions on them. I specifically see the query that this removes in π Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache() Active (search for workflows in the query list) and others might have hat it too of that meta, one that has a warm cache is likely more interesting as it has fewer queries in total.
But those issues have been stuck for a while and aren't trivial to get in due to their risk of random fails.
And it doesn't help that much to add explicit test coverage here, because those things are only really useful if they are already in HEAD and you can see the difference they make in a diff.
- π¨πSwitzerland berdir Switzerland
I did rebase the linked issue (and reopened the closed MR), and then merged in this MR, the resulting diff is:
$ git diff diff --git a/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php b/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php index 903c4fba006..9aaef449198 100644 --- a/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php +++ b/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php @@ -260,7 +260,6 @@ protected function testNodePageWarmCache(): void { 'SELECT "t".* FROM "node_revision__field_summary" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC', 'SELECT "t".* FROM "node_revision__field_tags" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC', 'SELECT "t".* FROM "node_revision__layout_builder__layout" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC', - 'SELECT "config"."name" AS "name" FROM "config" "config" WHERE ("collection" = "") AND ("name" LIKE "workflows.workflow.%" ESCAPE \'\\\\\') ORDER BY "collection" ASC, "name" ASC', 'SELECT "revision"."vid" AS "vid", "revision"."langcode" AS "langcode", "revision"."revision_uid" AS "revision_uid", "revision"."revision_timestamp" AS "revision_timestamp", "revision"."revision_log" AS "revision_log", "revision"."revision_default" AS "revision_default", "base"."nid" AS "nid", "base"."type" AS "type", "base"."uuid" AS "uuid", CASE "base"."vid" WHEN "revision"."vid" THEN 1 ELSE 0 END AS "isDefaultRevision" FROM "node" "base" INNER JOIN "node_revision" "revision" ON "revision"."nid" = "base"."nid" AND "revision"."vid" IN (75)', 'SELECT "revision".* FROM "node_field_revision" "revision" WHERE ("revision"."vid" IN (75)) AND ("revision"."vid" IN ("75")) ORDER BY "revision"."vid" ASC', 'SELECT "t".* FROM "node_revision__field_cooking_time" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC', @@ -286,7 +285,7 @@ protected function testNodePageWarmCache(): void { $this->assertSame($expected_queries, $recorded_queries); $expected = [ - 'QueryCount' => 172, + 'QueryCount' => 171, 'CacheGetCount' => 247, 'CacheGetCountByBin' => [ 'page' => 1,
- Status changed to Needs review
21 days ago 10:15pm 14 March 2025 - π¨πSwitzerland berdir Switzerland
That issue landed, so rebased, so now we can assert the improvement. It's not much, not compared with the amount of queries going on in that scenario, but every query saved helps.
The Needs Review Queue Bot β tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.