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

Adds a delegator to set a role for helpers #20

Open
wants to merge 1 commit into
base: 2.12.x
Choose a base branch
from

Conversation

froschdesign
Copy link
Member

@froschdesign froschdesign commented Apr 24, 2021

Q A
New Feature yes

Description

The delegator allows the setting of a role for navigation helpers by using a registered Laminas\Authentication\AuthenticationServiceInterface service and an existing identity.

Example of Usage

'navigation_helpers' => [
    'delegators' => [
        Laminas\View\Helper\Navigation::class => [
            Laminas\Navigation\View\RoleFromAuthenticationIdentityDelegator::class,
        ],
    ],
],

@froschdesign froschdesign force-pushed the feature/role-delegator branch from f02fb8b to 1a7ef45 Compare April 24, 2021 19:18
@froschdesign froschdesign linked an issue Apr 24, 2021 that may be closed by this pull request
@froschdesign
Copy link
Member Author

@Ocramius
I want to register this delegator so that it is present by default but this can then again be interpreted as a BC break.
What do you think?

@froschdesign
Copy link
Member Author

The same applies to Laminas\Db\Adapter\AdapterAwareTrait and the registration for the database validators in laminas-validator.

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO configuring this out of the box is a bit too much

$this->getRoleMethodName = $getRoleMethodName;
}

public static function __set_state(array $state): self
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is going on here? O_o

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for caching of the configuration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the config only contain the delegator name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not enough if serialization is used via var_export.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius you can either specify a class name in the delegator configuration, or any callable. We've had a few cases where we've created delegator factory classes that optionally accept constructor arguments, which then allow them to vary their behavior; in such cases, providing a __set_state implementation allows them to work with caching.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var_export does not serialize/deserialize objects. Instead objects are exported as a call to ObjectClassName::__set_state() with object state passed as a map [property => value]. This method does not have default implementation and so it must be provided for the object in configuration to be "serializable" for the config cache

$role = $identity->{$this->getRoleMethodName}();
}

$helper->setRole($role);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially broken, as it enforces temporal coupling between when the authentication storage has an identity (and role), and when the helper is built.

It's done in good faith, but IMO it is quite broken 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, this is done in the view layer: potentially OK, but only works in traditional php-fpm based apps, not long-running apps.

Copy link
Member Author

@froschdesign froschdesign Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially broken, as it enforces temporal coupling between when the authentication storage has an identity (and role), and when the helper is built.

Right, therefore the different checks before.

…but only works in traditional php-fpm based apps, not long-running apps.

Correct, but this applies to the view helpers in general.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the whole laminas-view layer is crazy stateful, which is a problem for things like Swoole/ReactPHP/AMPHP. I steer people away from laminas-view for those applications, as we can't really address it any other way currently without a full rewrite — and if we want to do that, I'd argue we instead consider dropping it for something like mezzio-template, so that devs can choose their own poison templating solution.

I think this is fine for the current architecture.

@froschdesign
Copy link
Member Author

IMO configuring this out of the box is a bit too much

The other idea is to create CLI command to register the delegator.

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@froschdesign Seems sane enough, given the lamians-view architecture.

Only thing missing for me is documentation of how to attach the delegator, and why you'd want to.

@froschdesign
Copy link
Member Author

@gsteel
Maybe it would be a better idea to use the plugin manager for the navigation view helper to inject the role and the ACL. What do you think?

https://github.com/laminas/laminas-view/blob/50ae82af3e2da96108490ceb474480e6df226993/src/Helper/Navigation/PluginManager.php#L23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation seems to be wrong ...
4 participants