laminas / laminas-navigation

Manage trees of pointers to web pages in order to build navigation systems
https://docs.laminas.dev/laminas-navigation/
BSD 3-Clause "New" or "Revised" License
17 stars 11 forks source link

Incorrect documentation for getUseAcl() #50

Open tyrsson opened 1 year ago

tyrsson commented 1 year ago

Bug Report

The documentation for:

    /**
     * Returns whether ACL should be used
     *
     * Implements {@link HelperInterface::getUseAcl()}.
     *
     * @return bool
     */
    public function getUseAcl()
    {
        return $this->useAcl;
    }

From the docs:

Whether or not to use ACLs; both the flag must be enabled and an ACL instance present.

Reference this page: https://docs.laminas.dev/laminas-navigation/helpers/intro/

This method does not consult hasAcl to determine if an ACL is present.

froschdesign commented 1 year ago

This method does not consult hasAcl to determine if an ACL is present.

The current description does not say that further checking will be done. However, a detailed explanation with code examples on this topic is still missing, see: #48

tyrsson commented 1 year ago

Then how is it supposed to satisfy this part?

both the flag must be enabled and an ACL instance present.

The use of the words both and there tells me that more than one value was to be checked against. Both means two, not one. So, it does, very much, say that more than just the useAcl flag was to be consulted. As stated in the doc it is incorrect.

My point is that the docs should not mention that an ACL has to be present. hasAcl is never consulted. For the docs to continue to state that it is deterministic for one to be present is 100% incorrect.

I must say that this discussion has now peaked my interest in who made the changes that added the RBAC support and the event handling used in the accept method.

froschdesign commented 1 year ago

My point is that the docs should not mention that an ACL has to be present. hasAcl is never consulted. For the docs to continue to state that it is deterministic for one to be present is 100% incorrect.

The description is addressed to the user and does not explain what the method does internally. But there is clearly a lack of an example here, because the explanation is too short and thus insufficient.

I must say that this discussion has now peaked my interest in who made the changes that added the RBAC support and the event handling used in the accept method.

I wouldn't waste time here because it was added after ACL support was already implemented. This was in the days of ZF but that does not help with the current problem and does not bring any further documentation.


Beside the improvement of the documentation, the goal is to extract the view helpers for laminas-navigation into a separate package so that improvements to the helpers itself can be made more quickly.

tyrsson commented 1 year ago

Beside the improvement of the documentation, the goal is to extract the view helpers for laminas-navigation into a separate package so that improvements to the helpers itself can be made more quickly.

That would be awesome. I've often wondered why those view helpers are not in the laminas-navigation package?

I wouldn't waste time here because it was added after ACL support was already implemented. This was in the days of ZF but that does not help with the current problem and does not bring any further documentation.

I have about 1,000 things that are higher on the priority list than this. I started using ZF around the 1.10 release or so but my memory is nowhere near good enough to recall changes from that far back ;)

froschdesign commented 1 year ago

I've often wondered why those view helpers are not in the laminas-navigation package?

Historical reasons; but the navigation helpers of laminas-view will not be added in this repository as this limits laminas-navigation to laminas-view.