Optimize menu_rebuild()

[hosef]

Reducing the number of database queries used when a menu link is saved or when the menus are rebuilt.

Fixes backdrop/backdrop-issues#5389

[indigoxela]

@hosef which issue does this PR refer to?

[hosef]

@indigoxela there isn't actually an issue. I initially wasn't even sure this would work. I just made the PR to see how the test bot would handle it.

[docwilmot]

This PR says it optimizes menu_rebuild() but the issue says menu_navigation_links_rebuild().

[hosef]

@docwilmot
When I initially created the PR, I was trying to make menu_rebuild() faster, but I didn't create an issue because I wasn't sure it would work or if anything would come of it. After finally getting it working, it turned out that most of my changes where in menu_navigation_links_rebuild(), so that is the function name I put in the issue title.

[jenlampton]

@hosef great work, I'm impressed you were able to remove so many database queries (and surprised there were so many to remove!)

I have one question, why were these tests removed?

   // Start over, forcefully delete child-1 from the database, simulating a
    // database crash. Check that the children of child-1 have been reassigned
    // to the parent, going up on the old path hierarchy stored in each of the
    // links.

[hosef]

@jenlampton Those tests were checking the behavior of the menu re-parenting when a different page uses a manual database query to remove a menu link in the middle of your page load.
I don't think this should be supported behavior because we have provided menu_delete as a supported method of removing a menu link. We also don't support that in any other context, such as checking that a node wasn't deleted in the middle of generating a view.

[jenlampton]

@hosef Does this change cause those tests to fail?

If not, I'd prefer that we remove those tests in a separate issue where we can discuss why it was supported in the first place, just in case there's something else we're missing. Including that decision in an optimization issue might be setting us up for unforeseen consequences and it makes me nervous :)

[hosef]

Those tests can't pass with the new code because they bypass the built in menu API functions, so they don't update the static cache used internally in menu_rebuild().

[jenlampton]

Ah ok! Let's see if other committers have any concerns. If not it's probably worth the risk :)

…

On Thu, Dec 23, 2021, 5:27 PM hosef ***@***.***> wrote: Those tests can't pass with the new code because they bypass the built in menu API functions, so they don't update the static cache used internally in menu_rebuild(). — Reply to this email directly, view it on GitHub <#3659 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBERY5O6M2MC7CMTZXYF3USPEADANCNFSM5AC4V53Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[dragonbot]

Tugboat has finished building a preview for this pull request!

Website: https://pr3659-uaathajauinc6qcrjnh2xjmjxi9vvsnu.tugboat.qa/
Username: admin
Password: 3ed79224f533

[quicksketch]

I've merged this into 1.x for inclusion in 1.21.0. Great work @hosef!