frankkessler / guzzle-oauth2-middleware

Guzzle middleware to perform oauth2 connections - Guzzle 6
MIT License
13 stars 14 forks source link

Allow extending stack handlers but preserve oauth middlewares #15

Open mirfilip opened 7 years ago

mirfilip commented 7 years ago

Currently, Oauth2Client constructor allows to completely override the default stack by specifying own handler but it won't add oauth-specific middlewares to passed handler.

Real life scenario I have is I need to add some header to all requests against some API. I need to do it both for main handler and for token_handler.

For token_handler, it's easy to create my own and pass as token_handler to constructor params of Oauth2Client, as the are no oauth specific middlewares added. I lose nothing. If I do the same to the main handler, I won't get oauth middlewares attached.

To me, it feels ill advised. When handler is specified, it'd be much better to take the handler and append oauth handlers. Of course, I can extend Oauth2Client and use protected returnHandlers to modify it, but it defeats the purpose of having configurable handler.

Please consider either:

  1. Changing the behaviour when handler option is specified (append oauth handlers instead of ignoring).
  2. Making returnHandlers public so that one can take oauth handlers stack and inject own middlewares.

EDIT: I can contribute with that but need your opinion.

frankkessler commented 7 years ago

@mirfilip Looking at it now, I can't see any reason why we shouldn't make returnhandlers a public method so you can extend the class and add handlers

ametad commented 7 years ago

The function is protected, so with extending the class the 'returnHandlers' function can already be used. But if you want to make a wrapper around Oauth2Client, then a public function would be perfect!

mirfilip commented 7 years ago

@ametad Of course, but the feature that allows passing custom handler with token_handler config option is useless if you don't append oauth2-specific middlewares to it. Default should either be to append middlewares to token_handler or just allow people to get oauth2 middlewares so that they can be set to custom token_handler.

Consider a difference between:

$myCustomTokenHandler = \GuzzleHttp\HandlerStack::create();
$myCustomTokenHandler->push(); // add my custom middleware - e.g. to log token calls or to persist them

$client = new \Frankkessler\Guzzle\Oauth2\Oauth2Client(
                [
                    'auth' => 'oauth2',
                    'token_handler' => $myCustomTokenHandler, // there is no middleware to acquire access token :(
                ]
            );

and

$myCustomTokenHandler = \Frankkessler\Guzzle\Oauth2\Oauth2Clien::returnHandlers(); // it contains all the oauth2 stuff now!
$myCustomTokenHandler->push(); // add my custom middleware - e.g. to log token calls or to persist them

$client = new \Frankkessler\Guzzle\Oauth2\Oauth2Client(
                [
                    'auth' => 'oauth2',
                    'token_handler' => $myCustomTokenHandler,
                ]
            );

Sure, you can extend the client but it's a pity to have the configuration option and still have go with inheritance.

ametad commented 7 years ago

Indeed looking good 👍 @mirfilip