Convert menu link SQL queries to the Entity Query API

I don't see any sql queries in the menu_link module (except the \Drupal\menu_link\MenuLinkStorageController of course).

So I think this issue should be closed.

Working on this

As discussed with amateescu.

Some queries will join menu_router table as well, some menu_router properties are not accessible and menu router should be gone (issue is not created yet).

It seems these few queries shouldn't be modified further than moving them to the menu link storage controller.

Here's an initial patch.

I've converted this enigmatic comment as well, not sure if there's something hidden behind it:

// @todo Convert this to an EFQ once we figure out 'ORDER BY m.number_parts'.

Also this:

// For performance reasons, do a straight query now and convert to a menu
// link entity later.
// @todo revisit before release.

I converted it but in the top of the function (_menu_navigation_links_rebuild) it stands:

@todo This function should be removed/refactored.

There are a couple of things left in book module queries menu_links table twice joining with the book table, as books are not entities (?), they don't have a storage controller, shall we put those queries (that actually return books) into the menu link storage controller?

7: 2068349.patch queued for re-testing.

Rerolled Patch of 2068349.patch comment # 7 by by pcambra on November 22, 2013 at 10:06pm. Applying properly on date.

Hi,

I've checked your patch, looks ok, but it might need some work. I found other menu_link queries in Drupal core here:

• BookManager::findChildrenRelativeDepth
• BookManager::updateId
• BookManager::BookMenuSubTreeData
• toolbar_get_rendered_subtrees

What do you think?

Ignore book manager, that will soon go away.

Hi ,
I have seen in core/module/toolbar/toolbar.module in toolbar_get_rendered_subtrees function the menu_link is rendered through EntityQuery() function. I am not sure how to proceed. However, When i try to merge my patch with latest 8.x head I got some merge conflict so I edited the patch and issued a new re-rolled patch. Please check and let me know how to proceed.

Previous patch was wrong

1. +++ b/core/modules/menu/lib/Drupal/menu/Form/MenuDeleteForm.php
   @@ -105,8 +105,12 @@ public function submit(array $form, array &$form_state) {
   +<<<<<<< HEAD
   +    $result = $this->storageController->getMenuLinksDefinedBySystem($this->entity->id());
   +=======
   

   Merge conflict should be solved.

2. +++ b/core/modules/menu/lib/Drupal/menu/Form/MenuDeleteForm.php
   @@ -105,8 +105,12 @@ public function submit(array $form, array &$form_state) {
   +>>>>>>> 0b66890345155c3165ef8090e367a4042360382e
   

   git conflict.

3. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageControllerInterface.php
   @@ -34,6 +34,45 @@ public function setPreventReparenting($value = FALSE);
   +<<<<<<< HEAD
   +   * Gets all the menu names that have expanded menu links.
   +   *
   +   * @return array
   +   *  An array of menu names.
   +   */
   +  public function getMenuNamesWithExpandedItems();
   +
   +  /**
   +   * Gets all the menu links defined by hook_menu in system module ordered
   +   * ascending by their menu_router.
   +   *
   +   * @param $menu_name
   +   *  Menu to filter by.
   +   *
   +   * @return array
   +   *   An array of menu link objects indexed by their ids and ordered by the
   +   *   menu_router number_parts column.
   +   */
   +  public function getMenuLinksDefinedBySystem($menu_name);
   +
   +
   +  /**
   +   * Loads updated and customized menu links for specific router paths.
   +   *
   +   * Note that this is a low-level method and it doesn't return fully populated
   +   * menu link entities. (e.g. no fields are attached)
   +   *
   +   * @param array $router_paths
   +   *   An array of router paths.
   +   *
   +   * @return array
   +   *   An array of menu link objects indexed by their ids.
   +   */
   +  public function loadUpdatedCustomized(array $router_paths);
   +
   +  /**
   +=======
   +>>>>>>> 0b66890345155c3165ef8090e367a4042360382e
   

   conflict again.

Thanx Ajit .. Fixed the conflict now

22: 2068349_9feb_solved.patch queued for re-testing.

Seems to me that #16 has introduced a change from elsewhere, not sure where some changes are coming from, here's a re roll starting in #11 and adapting the new code for menu_link_rebuild_defaults().

Here's a new reroll, removed one of the methods from the storage controllers as it is not needed anymore.

Yay, happy to see a small green patch!

1. +++ b/core/includes/menu.inc
   +++ b/core/includes/menu.inc
   @@ -1620,17 +1620,13 @@ function menu_link_rebuild_defaults() {
   

   Let's leave the query in this function in place because I'm not sure the performance problem mentioned in there has been fixed..

2. +++ b/core/includes/menu.inc
   @@ -1777,7 +1773,7 @@ function _menu_clear_page_cache() {
   -  $names = db_query("SELECT menu_name FROM {menu_links} WHERE expanded <> 0 GROUP BY menu_name")->fetchCol();
   +  $names = \Drupal::entityManager()->getStorageController('menu_link')->getMenuNamesWithExpandedItems();
   

   Entity query has GROUP BY (aggregate) support in D8 (https://drupal.org/node/1918702) so we should be able to use an entity query here instead of a new method on the storage controller :)

3. +++ b/core/includes/menu.inc
   index 3cdf898..9cc7ab3 100644
   --- a/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
   +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
   

   Unrelated changes, should probably be handled in another patch.

Thanks for the review @amateescu!

#1 & #3 have been removed from the patch, let's see how #2 looks, the array resultant is slightly different (it's keyed) but for what the use is I guess it doesn't matter...

Let's see what the testbot thinks

It seems it did matter, I had to add a way to convert the array into a flat one as the one coming from the entity query has the same key, menu name and it's not the way it's expected.

This looks ready to go. Thanks @pcambra for sticking with it :)

Committed 108789c and pushed to 8.x. Thanks!

Minor code style fixes on commit...

diff --git a/core/includes/menu.inc b/core/includes/menu.inc
index 120d8c2..3533c85 100644
--- a/core/includes/menu.inc
+++ b/core/includes/menu.inc
@@ -1783,8 +1783,8 @@ function _menu_set_expanded_menus() {
     ->groupBy('menu_name')
     ->execute();
 
-  // Flattern the resulting array.
-  foreach($result as $k=>$v) {
+  // Flatten the resulting array.
+  foreach($result as $k => $v) {
     $names[$k] = $v['menu_name'];
   }