Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of #2068325: [META] Convert entity SQL queries to the Entity Query API. See the parent issue for details.
Comment | File | Size | Author |
---|---|---|---|
#35 | 2068349-menu_link_sql-35.patch | 5.54 KB | pcambra |
#35 | interdiff.txt | 684 bytes | pcambra |
#32 | 2068349-menu_link_sql-32.patch | 5.41 KB | pcambra |
#32 | interdiff.txt | 4.16 KB | pcambra |
#29 | 2068349-menu_link_sql-29.patch | 8.95 KB | pcambra |
Comments
Comment #1
Désiré CreditAttribution: Désiré commentedI 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.
Comment #2
plachWe should also check outside of the menu_link module, it seems there's some stuff in the menu.inc file querying the menu_links table directly.
Comment #3
Désiré CreditAttribution: Désiré commentedComment #4
pcambraWorking on this
Comment #5
pcambraAs 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.
Comment #6
plachComment #7
pcambraHere's an initial patch.
I've converted this enigmatic comment as well, not sure if there's something hidden behind it:
Also this:
I converted it but in the top of the function (_menu_navigation_links_rebuild) it stands:
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?
Comment #8
pfrenssen7: 2068349.patch queued for re-testing.
Comment #11
swastik1608 CreditAttribution: swastik1608 commentedRerolled Patch of 2068349.patch comment # 7 by by pcambra on November 22, 2013 at 10:06pm. Applying properly on date.
Comment #12
swastik1608 CreditAttribution: swastik1608 commentedComment #13
skipyT CreditAttribution: skipyT commentedHi,
I've checked your patch, looks ok, but it might need some work. I found other menu_link queries in Drupal core here:
What do you think?
Comment #14
BerdirIgnore book manager, that will soon go away.
Comment #15
swastik1608 CreditAttribution: swastik1608 commentedHi ,
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.
Comment #16
swastik1608 CreditAttribution: swastik1608 commentedComment #17
swastik1608 CreditAttribution: swastik1608 commentedComment #19
swastik1608 CreditAttribution: swastik1608 commentedPrevious patch was wrong
Comment #21
AjitSMerge conflict should be solved.
git conflict.
conflict again.
Comment #22
swastik1608 CreditAttribution: swastik1608 commentedThanx Ajit .. Fixed the conflict now
Comment #23
swastik1608 CreditAttribution: swastik1608 commentedComment #25
jibran22: 2068349_9feb_solved.patch queued for re-testing.
Comment #27
pcambraSeems 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().
Comment #29
pcambraHere's a new reroll, removed one of the methods from the storage controllers as it is not needed anymore.
Comment #31
amateescu CreditAttribution: amateescu commentedYay, happy to see a small green patch!
Let's leave the query in this function in place because I'm not sure the performance problem mentioned in there has been fixed..
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 :)
Unrelated changes, should probably be handled in another patch.
Comment #32
pcambraThanks 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
Comment #35
pcambraIt 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.
Comment #36
amateescu CreditAttribution: amateescu commentedThis looks ready to go. Thanks @pcambra for sticking with it :)
Comment #37
alexpottCommitted 108789c and pushed to 8.x. Thanks!
Minor code style fixes on commit...