I have tested this item unsuccessfully on 51de1eb

I get this php notices:

Notice: Undefined property: stdClass::$rgt in /path/to/joomla-cms/modules/mod_menu/helper.php on line 77
Notice: Undefined property: stdClass::$lft in /path/to/joomla-cms/modules/mod_menu/helper.php on line 77

@Hackwar, instead of doing this:

foreach ($items as $i => $item)
{
    /* Code removed for simplifying */
    $item->deeper     = false;
    $item->shallower  = false;
    $item->level_diff = 0;

    if (isset($items[$lastitem]))
    {
        $items[$lastitem]->deeper     = ($item->level > $items[$lastitem]->level);
        $items[$lastitem]->shallower  = ($item->level < $items[$lastitem]->level);
        $items[$lastitem]->level_diff = ($items[$lastitem]->level - $item->level);
    }

    $item->parent = (boolean) ($item->rgt > $item->lft + 1) && $menu->getItems('parent_id', (int) $item->id, true);
    /* Code removed for simplifying */
}

if (isset($items[$lastitem]))
{
    $items[$lastitem]->deeper     = (($start?$start:1) > $items[$lastitem]->level);
    $items[$lastitem]->shallower  = (($start?$start:1) < $items[$lastitem]->level);
    $items[$lastitem]->level_diff = ($items[$lastitem]->level - ($start?$start:1));
}

Can't we do something like this?:

foreach ($items as $i => $item)
{
    /* Code removed for simplifying */
    $item->deeper     = false;
    $item->shallower  = false;
    $item->level_diff = 0;
    $item->parent     = 0;

    if (isset($items[$lastitem]))
    {
        $items[$lastitem]->deeper     = ($item->level > $items[$lastitem]->level);
        $items[$lastitem]->shallower  = ($item->level < $items[$lastitem]->level);
        $items[$lastitem]->level_diff = ($items[$lastitem]->level - $item->level);
        $items[$lastitem]->parent     = ($items[$lastitem]->level_diff == -1);
    }
    /* Code removed for simplifying */
}

if (isset($items[$lastitem]))
{
    $items[$lastitem]->deeper     = (($start?$start:1) > $items[$lastitem]->level);
    $items[$lastitem]->shallower  = (($start?$start:1) < $items[$lastitem]->level);
    $items[$lastitem]->level_diff = ($items[$lastitem]->level - ($start?$start:1));
    $items[$lastitem]->parent     = ($items[$lastitem]->level_diff == -1);
}

In other words, can't we just don't use the getItems to calculate if current menu item is a parent or not and use the already calculated level_diff to do that.

If we can do that we get even further performance improvements.

I run a test with a menu with several levels (thank for batch processing :)) and the for cycle got from:

• 25 published items: ~6.5 ms to ~4.7 ms.
• 50 published items: ~15 ms to ~9 ms.
• 100 published items: ~30 ms to ~19 ms.
• 200 published items: ~78 ms to ~38 ms.
• 400 published items: ~232 ms to ~76 ms.
• 800 published items: ~730 ms to ~155 ms.
• and so on ...

This PR has received new commits.

CC: @andrepereiradasilva

First of all, sorry for the broken code. Yes, you are right, your proposal is even better. I added your code. I'm setting the parent to false by default to have it set at all times.

I have tested this item successfully on df6acd2

Thank you for find that needed performance optimization.
This can have some performance impact on sites with a lot of menu items.

Correct me if i'm wrong but the last element can't be the parent of anything, since it's the last.
That is why you didn't put the $items[$lastitem]->parent = ($items[$lastitem]->level_diff == -1); in the setting of last item after the for cycle as i suggested.
Makes sense.

I have tested this PR unsuccessfully on df6acd2.

Simple test, I take default installation added 5 Menu Items:

• Item 1
  • Item 1a
• Item 2
  • Item 2a
    • Item 2b

Module with "Show Sub-menu Items" off shows on active "Item 1" a difference at the last item, in this case "Item 2".

Without patch: parent = true
With patch: parent = false

If "Show Sub-menu Items" is on the "Item 2" parent value is correct (true).

Also if "Show Sub-menu Items" is on the the complete testcase is correct.

@efEris we know the last item parent value can be not ok. See my comment when i tested.

Correct me if i'm wrong but the last element can't be the parent of anything, since it's the last.
That is why you didn't put the $items[$lastitem]->parent = ($items[$lastitem]->level_diff == -1); in the setting of last item after the for cycle as i suggested.
Makes sense.

But in what that affected your test? Did the menu not appeared correctly?

@andrepereiradasilva sure because the li-tag for the last element doesn't get the "parent" class, that could lead to a visual problem if you use this class (the only way) to mark the link as parent item.

Ok, let's wait for @Hackwar to solve that.

This PR has received new commits.

CC: @andrepereiradasilva

This PR has received new commits.

CC: @andrepereiradasilva

Please test this again. This should solve the previous issues.

Hi,

while testing this I think I came to a better solution.

Add this to the beginning of the foreach loop before filtering the unseen items.

$item->parent = false;
if (isset($items[$lastitem])
  && $items[$lastitem]->id == $item->parent_id)
{
  $items[$lastitem]->parent = true;
}

btw. is it normal that the joomla patch tester overrides the complete file with a incompatible version?

I think the prior solution (before revert) works, if you add this line inside the if after the for cycle (https://github.com/Hackwar/joomla-cms/blob/df6acd259cb3062ad0ec1b1499f72aa6cea3e10e/modules/mod_menu/helper.php#-L126-L131) to set the last parent:

$items[$lastitem]->parent     = ($items[$lastitem]->level_diff == -1);

@andrepereiradasilva I tested this first, but no that will not work because we don't have the child at this point. if you can please try my solution.

ok i will check. thanks

@efEris i tried you solution and it worked. And without the getItems. Good work.

How did i test:

• Use the current php file (before this PR) and removed this line: https://github.com/joomla/joomla-cms/blob/staging/modules/mod_menu/helper.php#L85.

• As you said, put your code in this line https://github.com/joomla/joomla-cms/blob/staging/modules/mod_menu/helper.php#L56, so it becomes

foreach ($items as $i => $item)
{
    $item->parent = false;
    if (isset($items[$lastitem]) && $items[$lastitem]->id == $item->parent_id)
    {
        $items[$lastitem]->parent = true;
    }
    // code removed for brevity
}

@Hackwar can you check? With @efEris solution i think we can get rid of getItems and with that improving performance even further.

@hackwar any comment on the proposed improvement from @efEris would be good to move this PR forward if we can

@Hackwar Another nudge so we can move this PR forward.

I don't know which one is @efEris. So far this PR is complete the way it is.

@Hackwar sorry I renamed the github Username

Instead of trying to get the ->parent value based on the level_diff its better to set it at the beginning before we delete the information.

Add the following code to the beginning of the foreach loop at Line 57, see comment from @andrepereiradasilva

    $item->parent = false;
    if (isset($items[$lastitem]) && $items[$lastitem]->id == $item->parent_id)
    {
        $items[$lastitem]->parent = true;
    }`

This PR has received new commits.

CC: @andrepereiradasilva

I've added the change like you proposed @HLeithner.

This PR has received new commits.

CC: @andrepereiradasilva

One thing, adding ", m.lft, m.rgt" to the query is no longer necessary.

This PR has received new commits.

CC: @andrepereiradasilva

One thing, adding ", m.lft, m.rgt" to the query is no longer necessary.

Menu with 500+ items

Before patch

After patch

So aprox. half the time

I have tested this item ✅ successfully on f1aa5a6

as comment above

I have tested this item ✅ successfully on c4d5c8c

I have tested this on a stock Joomla distro (3.6.3) will "Learning Sample data" in 4 languages and it has a visible faster loading time compared to pre-patch

This issue has two successful tests.

RTC

Milestone changed to 3.7 as it is now not planned to have a 3.6.4 release