Bug: Navigation Helper - isAllowed function - ACLlistener is Attached Several Times

[cgaube]

Here is a fun bug for you all.

Everytime you instantiate a new Navigation Helper the the same Listener is attached to the shared Manager once again.

file: https://github.com/zendframework/zend-view/blob/master/src/Helper/Navigation/AbstractHelper.php

protected function setDefaultListeners()
    {
        if (! $this->getUseAcl()) {
            return;
        }
        $events = $this->getEventManager() ?: $this->createEventManager();
        if (! $events->getSharedManager()) {
            return;
        }
        $events->getSharedManager()->attach(
            'Zend\View\Helper\Navigation\AbstractHelper',
            'isAllowed',
            ['Zend\View\Helper\Navigation\Listener\AclListener', 'accept']
        );
    }

so if you do

echo $this->navigation('Name')->menu();
echo $this->navigation('Name')->breadcrumbs();

Then the same AclListener function will be attached 3 times

• one on ->navigation
• another on ->menu
• and the last on -> breadcrumbs

It means that everytime the "isAllowed" function is called (it is called a LOT) then the AclListener function is called X times (3 times in my example).........

To fix this issue I believe that Zend View should not be in charge of registering any event
So my fix would be to get rid of this function altogether.
The Acl Listener should be moved the the zend-acl repository

And it should be the user responsibility to register any event on the isAllowed function.

[cgaube]

Here is my workaround that I absolutely hate

In one of my module I am adding a delegator at the "navigation" view helper level

'view_helpers' => [
   'delegators' => [
           // Navigation isAllowed Bug Fix
            \Zend\View\Helper\Navigation::class => [
                Navigation\ServiceManager\NavigationHelperIsAllowedBugFixDelegator::class
            ]
    ]
]

Here is the delegator:

use Zend\ServiceManager\Factory\DelegatorFactoryInterface;
use Interop\Container\ContainerInterface;

class NavigationHelperIsAllowedBugFixDelegator implements DelegatorFactoryInterface
{

    public function __invoke(ContainerInterface $container, $name, callable $callback, array $options = null)
    { 
        $eventManager = $container->get('EventManager');
        $this->eventManager = $eventManager;
        
        /* @var $helper \Zend\View\Helper\Navigation */
        $helper = $callback();
        $this->attachEventListenerWithoutAcl($helper);
        

        /* @var $pluginManager \Zend\View\Helper\Navigation\PluginManager */
        $pluginManager = $helper->getPluginManager();
        
        // We Cannot use Initializer because our problem appears on the setEventManager initializer 
        // that is registered by the Navigation Plugin Manager construct function
        // and there is not way to add an initializer at the top of the queue
        // So the workaround is to add a delegator - for all the default navigation Helpers.       
        $plugins = [
            \Zend\View\Helper\Navigation\Menu::class,
            \Zend\View\Helper\Navigation\Breadcrumbs::class,
            \Zend\View\Helper\Navigation\Sitemap::class,
            \Zend\View\Helper\Navigation\Links::class,
        ] ;
        
        foreach($plugins as $pluginName){
            $pluginManager->addDelegator($pluginName, function(ContainerInterface $container, $name, $callback) use($eventManager){
                $helper = $callback();
                $this->attachEventListenerWithoutAcl($helper);
                return $helper;
            });
        }
        
        return $helper;
    }

    public function attachEventListenerWithoutAcl($helper){
        $oldValue = $helper->getUseAcl();
        
        $helper->setUseAcl(false);
        $helper->setEventManager($this->eventManager);
        $helper->setUseAcl($oldValue);
        return $helper;
    }
    
}

Because it is a delegator this code is executed before any initializers.
So what it does is

Get the navigation helper
tell him not to use Acl (setUseAcl(false) );
inject the eventManager (and because setAcl is false it means that no listeners will be added to the Shared Event Manager)

and tell him to reuse Acl - so that the event is actually triggered in the "accept" function

Now the ugly part is I add another delegator on all the Navigation Plugin Manager level for each Navigation Helpers

• menu
• breadcrumbs
• sitemaps
• link

This delegator does the same thing than above

It has to be delegators and cannot be initializers because the issue comes from the Initializer added by the Navigation View Helper Plugin Manager (in the __construct function)

So I hate this workaround - but I don't think we can do it another way.

[cgaube]

Also it would be nice if the the isAllowed function had some kind of caching mechanism at the navigation container level - so it does not have to trigger events over and over if a page has already been accepted before

By the way - I would be more than happy to do the work if needed -
But I suppose it needs some kind of approval or something ? let me know.

[cgaube]

Another improvement:
Stop the propagation of events listening to the isAllowed event when one returns false.
If one listener returns false it means that the page is not allowed and thus we can safely stop the propagation...

Zend\View\Helper\Navigation\AbstractHelper

    /**
     * Determines whether a page should be allowed given certain parameters
     *
     * @param   array   $params
     * @return  bool
     */
    protected function isAllowed($params)
    {
        $events = $this->getEventManager() ?: $this->createEventManager();
        $results = $events->triggerUntil(function($result){
            return $result === false;
        }, __FUNCTION__, $this, $params);
      
        return $results->last();
    }

[froschdesign]

@aft-christophe

To fix this issue I believe that Zend View should not be in charge of registering any event

This will be addressed for version 3 of zend-navigation. And also other performance improvements.

The Acl Listener should be moved the the zend-acl repository

Why does zend-permissions-acl need to know something from zend-navigation? (Do not forget zend-permissions-rbac.)

[cgaube]

Thank you

This will be addressed for version 3 of zend-navigation. And also other performance improvements.

That s great.

Why does zend-permissions-acl need to know something from zend-navigation? (Do not forget zend-permissions-rbac.)

Ok so because it is related to navigation you want that in the navigation repo - that s fine by me.

We could ask the question the other way around though - why would zend-navigation need to
know something from zend-permissions-acl

Thanks

[froschdesign]

@aft-christophe

…you want that in the navigation repo…

I never said that! 😃

Btw. I think, before version 3 we can add some small improvements like stopping the propagation.

[weierophinney]

This repository has been closed and moved to laminas/laminas-view; a new issue has been opened at laminas/laminas-navigation-view#7.