looking at potential uses of hook_translated_menu_link_alter() I realized that we might have an easy performance patch for D6 and D7. It really seems pointless (and potentially a lot of extra overhead) to localize tabs or links that we will not be displayed.

CommentFileSizeAuthor
#9 266596_remove_notice_messages-2-7.x.patch982 bytesdanillonunes
#7 266596_remove_notice_messages-7.x.patch980 bytesdanillonunes
#7 266596_remove_notice_messages-7.x.patch980 bytesdanillonunes
#7 266596_remove_notice_messages-6.x.patch822 bytesdanillonunes
#7 266596_remove_notice_messages-7.x.patch980 bytesdanillonunes
#1 menu-performance-266596-1.patch1.11 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
menu-performance-266596-1.patch1.11 KB

This (untested) patch is against D6, but should apply to D7 too.

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 7.x-dev » 6.x-dev

Committed to CVS HEAD.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

so I guess this is RTBC for 6.x too?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Reviewed & tested by the community
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Committed to Drupal 6, thanks!

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

danillonunes’s picture

danillonunes CreditAttribution: danillonunes commented
Version: 6.x-dev » 7.x-dev
Assigned: pwolanin » danillonunes
Status: Closed (fixed) » Needs review
FileSize
266596_remove_notice_messages-7.x.patch980 bytes
266596_remove_notice_messages-6.x.patch822 bytes
266596_remove_notice_messages-7.x.patch980 bytes
266596_remove_notice_messages-7.x.patch980 bytes

As I have PHP E_ALL errors enabled, this check keep showing me boring "Notice: Undefined index: access" messages on 7.x-dev. Cannot reproduce on 6.x-dev, but maybe is because there's no menu item without any "access" key. Anyway, I made a patch for both 7.x and 6.x.

Edit: Oh, sorry for a lot of patches attached... It's a bug with Filefield + Opera :-(.

bfroehle’s picture

bfroehle CreditAttribution: bfroehle commented 
+++ b/includes/menu.incundefined
@@ -1441,7 +1441,7 @@ function _menu_tree_check_access(&$tree) {
+    if (array_key_exists('access', $item) && $item['access'] || ($item['in_active_trail'] && strpos($item['href'], '%') !== FALSE)) {

For clarity, shouldn't that be
if( (A && B) || (C && D) !== FALSE )
not
if ( A && B || (C && D) !== FALSE )

Powered by Dreditor.

danillonunes’s picture

danillonunes CreditAttribution: danillonunes commented
FileSize
266596_remove_notice_messages-2-7.x.patch982 bytes

I just figure out that I had an active module whose files has been deleted, so that's why "access" key doesn't exists. I don't know if Drupal normally try to deal with these edge cases - maybe deleting files without uninstalling a module can give a lot of other troubles too because is an non recommended pratice. Anyway, I rerolled the first patch with #8 suggestion.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Version: 7.x-dev » 8.x-dev
kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
CreditAttribution: kscheirer commented

#9: 266596_remove_notice_messages-2-7.x.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 266596_remove_notice_messages-2-7.x.patch, failed testing.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Assigned: danillonunes » Unassigned
Issue summary: View changes
rahul.shinde’s picture

rahul.shinde
he/him
Primary language Marathi
Location Pune
CreditAttribution: rahul.shinde commented
Assigned: Unassigned » rahul.shinde
Issue tags: +#punedrupalgroup #SprintWeekend2015
rahul.shinde’s picture

rahul.shinde
he/him
Primary language Marathi
Location Pune
CreditAttribution: rahul.shinde commented
Assigned: rahul.shinde » Unassigned
Status: Needs work » Needs review

@mgifford, @kscheirer, @danillonunes I suppose the required changes are in the DefaultMenuLinkTreeManipulators class. Please help confirm this.

piyuesh23’s picture

piyuesh23 CreditAttribution: piyuesh23 commented
Issue tags: -#punedrupalgroup #SprintWeekend2015 +#punedrupalgroup, +#SprintWeekend2015
piyuesh23’s picture

piyuesh23 CreditAttribution: piyuesh23 commented
Issue tags: -#SprintWeekend2015 +SprintWeekend2015
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Use !empty() don't use array_key_exists()

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Title: menu performance - don't localize items where access is FALSE » Fix nocies in menu ehcn checking whether to localize items where access is FALSE
Status: Needs review » Needs work
Issue tags: +Needs issue summary update
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Title: Fix nocies in menu ehcn checking whether to localize items where access is FALSE » Fix notices in menu whenn checking whether to localize items where access is FALSE
Version: 8.0.x-dev » 7.x-dev

I also suspect this is no longer relevant for Drupal 8.0.x.

From \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkAccess()

   * Removes menu links from the given menu tree whose links are inaccessible
   * for the current user, sets the 'access' property to TRUE on tree elements
   * that are accessible for the current user.
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Fix notices in menu whenn checking whether to localize items where access is FALSE » Fix notices in menu when checking whether to localize items where access is FALSE

  • Dries committed a1537c3 on 8.3.x 
    - Patch #266596 by pwolanin: menu system performance improvement.

  • Dries committed a1537c3 on 8.3.x 
    - Patch #266596 by pwolanin: menu system performance improvement.

  • Dries committed a1537c3 on 8.4.x 
    - Patch #266596 by pwolanin: menu system performance improvement.

  • Dries committed a1537c3 on 8.4.x 
    - Patch #266596 by pwolanin: menu system performance improvement.

  • Dries committed a1537c3 on 9.1.x 
    - Patch #266596 by pwolanin: menu system performance improvement.