BreadcrumbMenu has serious performance issues

[kitsunet]

Since BreadcrumbMenu extends the regular Menu it behaves the same, that means looking at the MenuImplementation it will fetch "subItems" for documents in the breadcrumb. This is unexpected and probably unnecessary for 99% of all use cases, and results in serious performance issues if pages in the breadcrumb have many sub pages.

[kitsunet]

Currently a way to avoid this is to just replicate the EEL expression from the BreadcrumbMenu.ts2 (.fusion) and use a simple Collection or Template object to render this.

[kdambekalns]

Funny, I recently built

    /**
     * Prepare the menu item with state and label (and ignore sub items).
     *
     * @param NodeInterface $currentNode
     * @return array
     */

because the sub-item rendering broke something I relied on (for a completely different purpose)…

[kitsunet]

Fixing this is pretty much a breaking change as the subItems are expected, so IF people use it (in a Breadcrumb) it would break and I don't see an easy way to change that...

[aertmann]

Would prefer to fix this as a performance improvement, but maybe we have to wait until 4.0 and mark it a breaking change, that should only affect very few if any.

[kdambekalns]

This is the code I mentioned above:

<?php
declare(strict_types=1);

namespace Acme\Com\Fusion;

use Neos\ContentRepository\Domain\Model\NodeInterface;
use Neos\Neos\Fusion\MenuItemsImplementation;

/**
 * A Fusion MenuItems object
 */
class BreadcrumbMenuItemsImplementation extends MenuItemsImplementation
{
    /**
     * Prepare the menu item with state and label (and ignore sub items).
     *
     * @param NodeInterface $currentNode
     * @return array
     * @throws UnicodeException
     */
    protected function buildMenuItemRecursive(NodeInterface $currentNode)
    {
        if ($this->isNodeHidden($currentNode)) {
            $item = null;
        } else {
            $item = [
                'node' => $currentNode,
                'state' => self::STATE_NORMAL,
                'label' => $currentNode->getLabel(),
                'menuLevel' => $this->currentLevel,
                'linkTitle' => $currentNode->getLabel()
            ];

            $item['state'] = $this->calculateItemState($currentNode);

            $item['state'] = self::STATE_CURRENT;
        }

        return $item;
    }
}

[mhsdesign]

I would expect the Breadcrumb to use getNodesOnPath and be done with it ...

$node->getContext()->getNodesOnPath($siteNode, $documentNode);

that seems fairly performant ...

[kdambekalns]

Doh! Does the BreadcrumbMenu still use this? Yes, it does! 😱

Do we create a breaking change for v9 or do we fix it differently for v7+?

[mhsdesign]

I guess there is no fix in it for v7 ... as this would be a downgrade

[Sebobo]

We could add a toggle as fix to MenuItemsImplementation, then we also don't need an additional implementation. Either we enable it and add a note to the release notes or we have it disabled for 7.3 and people have to switch if needed.