Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect documentation for getUseAcl() #50

Open
tyrsson opened this issue May 25, 2023 · 5 comments
Open

Incorrect documentation for getUseAcl() #50

tyrsson opened this issue May 25, 2023 · 5 comments
Labels
Bug Something isn't working

Comments

@tyrsson
Copy link

tyrsson commented May 25, 2023

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.

@tyrsson tyrsson added the Bug Something isn't working label May 25, 2023
@froschdesign
Copy link
Member

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
Copy link
Author

tyrsson commented May 27, 2023

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
Copy link
Member

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
Copy link
Author

tyrsson commented May 27, 2023

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants