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

OAuth2ServerFactory is not returning instance of OAuth2\Server #14

Open
weierophinney opened this issue Dec 31, 2019 · 5 comments
Open

Comments

@weierophinney
Copy link
Contributor

The ZF\OAuth2\Factory\OAuth2ServerFactory is no longer returning a OAuth2\Server instance?

https://github.com/zfcampus/zf-oauth2/blob/master/src/Factory/OAuth2ServerFactory.php

The file has been changed in the latest version and it returns a closure (Factory!?) now. My code is broken since I simply used $serviceLocator->get('ZF\OAuth2\Service\OAuth2Server'); elsewhere to get the OAuth2\Server instance from the server manager.

The documentation has also not been updated accordingly and is no longer up to date since it still states it will give me an OAuth\Server.

Is returning another factory from a factory really the way to go? How should I now get my oauth server instance? Is it maybe possible to separate the keys so getting the server is still possible?


Originally posted by @Wilt at zfcampus/zf-oauth2#114

@weierophinney
Copy link
Contributor Author

My code editor also doesn't agree on a closure being returned by ServiceManager :)
I get big fat red lines under my DefaultAuthenticationListenerFactory class.

Deep down in the ZF2 ServiceLocatorInterface the return values from a service manager should be either an object or an array:

    /**
     * Retrieve a registered instance
     *
     * @param  string  $name
     * @throws Exception\ServiceNotFoundException
     * @return object|array
     */
    public function get($name);

In the ZF\MvcAuth\Factory\DefaultAuthenticationListenerFactory the following is done:

return new OAuth2Adapter($factory()); (https://github.com/zfcampus/zf-mvc-auth/blob/master/src/Factory/DefaultAuthenticationListenerFactory.php#L105)

That is why I get the following error comment:
Function name must be callable - a string, Closure or class implementing _invoke, currently array |object

I also get an error here

return new OAuth2Adapter($serverFactory(null)); (https://github.com/zfcampus/zf-mvc-auth/blob/master/src/Factory/DefaultAuthenticationListenerFactory.php#L128)

Since the php doc is not correct it looks like we are instantiating OAuth2\Server class here which is not possible.

I think it would be better to return the OAuth2\Server instead of the factory here or at least make sure all documentation is up to date so that it makes sense to my SDK.
I can make a PR if necessary.


Originally posted by @Wilt at zfcampus/zf-oauth2#114 (comment)

@weierophinney
Copy link
Contributor Author

@Wilt PR ? :)


Originally posted by @jguittard at zfcampus/zf-oauth2#114 (comment)

@weierophinney
Copy link
Contributor Author

@jguittard I was awaiting response from someone working on this repository to get some feedback on how to improve this...

@weierophinney Do you have time to take a quick look and make a suggestion on how to deal with this in a PR?

IMHO it would be good if this essential part of the zf-mvc-auth module becomes a bit more readable/understandable... All these $factory calls inside other factories are a bit ambiguous. As I mentioned before my code editor also doesn't agree with it:

image


Originally posted by @Wilt at zfcampus/zf-oauth2#114 (comment)

@weierophinney
Copy link
Contributor Author

@Wilt This was done due to issues with the design of oauth2-server. Essentially, the OAuth2\Server instance does work on initialization that can lead to issues with the HTTP request, database connections, and more. We had to provide a way to lazy-load it to solve the problems.

We can maybe solve the IDE issues with typehints, and some of these may be solved now (I did some work on the AuthControllerFactory earlier today). But I'm not sure we can make any other changes unless we change out the oauth2 server infrastructure or get changes pushed upstream to oauth2-server-php.


Originally posted by @weierophinney at zfcampus/zf-oauth2#114 (comment)

@weierophinney
Copy link
Contributor Author

@weierophinney Thanks for the explanation.

Would it be possible to use a lazy services pattern (using delegators) for this instead of this invokable factory? Or is this not sufficient to solve those issues you refer to?


Originally posted by @Wilt at zfcampus/zf-oauth2#114 (comment)

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

No branches or pull requests

1 participant