Contrib.social

Avoid extra DB query per menu item

Open on Drupal.org →
Created on 3 December 2021, almost 3 years ago
Updated 3 July 2023, over 1 year ago

Problem/Motivation

The translatable_menu_link_uri_iterate_menu() function loads MenuLinkContent entities by their UUID, which actually triggers a database query for every content link displayed on a page, even if the entity was already loaded by ID.

Steps to reproduce

  • Install Translatable menu link uri module.
  • Install Web Profiler sub-module of the Devel project.
  • Create a menu and add multiple content links there.
  • Put a block with this menu to the page.
  • Visit the page with the menu block visible.
  • Check the database log for queries like the following:
SELECT "base_table"."revision_id" AS "revision_id", "base_table"."id" AS "id" 
FROM "menu_link_content" "base_table" 
INNER JOIN "menu_link_content" "menu_link_content" ON "menu_link_content"."id" = "base_table"."id" 
INNER JOIN "menu_link_content_data" "menu_link_content_data" ON "menu_link_content_data"."id" = "base_table"."id" 
WHERE ("menu_link_content"."uuid" IN (:db_condition_placeholder_0)) 
AND ("menu_link_content_data"."default_langcode" IN (:db_condition_placeholder_1))

Proposed resolution

Not yet, see remaining tasks.

Remaining tasks

It's up to module maintainer to choose the approach here. Possible options are:

  • The \Drupal\menu_link_content\Plugin\Menu\MenuLinkContent::getEntity() does a very nice job of loading all the items by ID at once and it's better to re-use the method. However, this method is not open to public. Please check ✨ Set MenuLinkContent getEntity to public visibility Fixed .
  • Alternative approach would be to explore the MenuLinkContent definition, find entity ID saved there at $this-pluginDefinition['metadata']['entity_id'] and try to load the entity by ID. This way entity storage is able to take it from the static cache and it eliminates the extra query too. It could also collect entity IDs from all the menu items and then load them at once by IDs. However, this adds dependency on some internals of the Menu Link Content module and backward compatibility doesn't seem to be guaranteed for such things.

User interface changes

No.

API changes

No.

Data model changes

No.

🐛 Bug report
Status

Needs work

Version

2.0

Component

Code

Created by

🇵🇹Portugal dmitriy.trt

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment over 1 year ago →
    🇺🇸United States angrytoast Princeton, NJ

    The core issue ✨ Set MenuLinkContent getEntity to public visibility Fixed to expose \Drupal\menu_link_content\Plugin\Menu\MenuLinkContent::getEntity() looks like it will finally happen which makes Option 1 the best approach.

    This is to update the patch to work with the latest 2.x branch code. It also adds a more specific check for MenuLinkContentInterface instead of just EntityInterface because we are looking for a menu link content entity.

  • Comment over 1 year ago →
    🇺🇸United States angrytoast Princeton, NJ

    After experimenting a bit, it looks like we can further simplify code with a public MenuLinkContent::getEntity(). The entity retrieved via the method is already the correct translation, so we can skip the entity.repository call.

    Adding an updated patch with the simplified logic.

  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States angrytoast Princeton, NJ
  • Comment over 1 year ago →
    🇩🇪Germany Anybody Porta Westfalica

    Thank you, that's looking really good! I'm a bit afraid of things breaking, so would be nice to have automated test coverage. Then we can RTBC and merge this with confidence!

    And perhaps use a MR instead?

  • Comment over 1 year ago →
    🇷🇴Romania bboty

    I've tested #4 https://www.drupal.org/files/issues/2023-07-06/translatable_menu_link_ur... →

    and there is an issue. It's calling a protected method "getEntity()" that causes WSOD.

    $url = $link->getEntity()->link_override->first();
  • Comment about 1 year ago →
    🇪🇸Spain eduardo morales alberti Spain, 🇪🇺

    Eduardo Morales Alberti → made their first commit to this issue’s fork.

  • Comment 12 days ago →
    System Message
    • jsobiecki → committed c2a0f1c5 on 2.x 
      Issue #3252404 by angrytoast, eduardo morales alberti, dmitriy.trt,...
  • Status changed to Fixed 12 days ago
  • Comment 12 days ago →
    🇵🇱Poland jsobiecki Wroclaw

    I really like this change! The code looks much simpler and the fact that we reduced SQL queries number of site is very important.

    I share concern of @Anybody, that we may fall into some of regressions and unit tests can help reduce that risk. But I'd create a initial unit (rather kernel) test as separate tasks. This module was designed as quick and simple workaround in D8 times and I wanted to keep it that way. As we have D11 and it's still helpful, it's good idea to improve its quality. But not now, I'll do that as separate issue.

    I did some small refactor of code, as patch failed to apply and did manual testing. I was no able to "break" module after this modification.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024