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

Add OAuthService #964

Merged
merged 10 commits into from
Jul 6, 2020
Merged

Add OAuthService #964

merged 10 commits into from
Jul 6, 2020

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Jun 30, 2020

r? @ob-stripe
cc @stripe/api-libraries

Fixes #958 and adds $stripe->oauth with authorizeUrl, token, and deauthorize methods.

I chose to introduce ->requestConnect into the StripeClientInterface, rather than overload ->request, because to my intuition this seems cleaner, but I'm interested in second opinions.

@richardm-stripe richardm-stripe force-pushed the richardm-add-oauth-service branch 3 times, most recently from aa89bfd to 1e70240 Compare June 30, 2020 20:14
Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

I would prefer if the requestConnect stuff was kept inside OAuthService, since it's OAuth specific and very unlikely to be used by any other services in future.

lib/Util/RequestOptions.php Outdated Show resolved Hide resolved
lib/Service/OAuthService.php Outdated Show resolved Hide resolved
lib/Service/OAuthService.php Outdated Show resolved Hide resolved
lib/Service/OAuthService.php Outdated Show resolved Hide resolved
@richardm-stripe
Copy link
Contributor Author

ptal @ob-stripe -- I've addressed all your comments & moved 'connectRequest' out into OAuthService. Thanks for the careful review.

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

A few more comments, but this is looking great already

lib/BaseStripeClient.php Outdated Show resolved Hide resolved
lib/Service/OAuthService.php Outdated Show resolved Hide resolved
lib/Service/OAuthService.php Outdated Show resolved Hide resolved
lib/Service/OAuthService.php Outdated Show resolved Hide resolved
@richardm-stripe
Copy link
Contributor Author

ptal @ob-stripe

  1. Removed unnecessary calls to ->buildPath for constant paths.
  2. Changed $defaultOptions back to private.
  3. Changed ->authorizeUrl to use api_base instead of connect_base -- I refactored this into $opts->apiBase = $this->_getBase() function.
  4. I made ->_getBase() throw an exception if connect_base is passed. Might save a user a few cycles when switching from \Stripe\OAuth.

lib/Service/OAuthService.php Show resolved Hide resolved
// Throw an exception for the convenience of anybody migrating to
// \Stripe\Service\OAuthService from \Stripe\OAuth, where `connect_base`
// was the name of the parameter that behaves as `api_base` does here.
if (isset($opts->connect_base)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this will work. \Stripe\RequestOptions does not have a connect_base property, even if there is a connect_base key in the array passed to RequestOptions::parse(), so I think this condition will always be false. Mind adding a test for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Good catch. Fixed now.

@richardm-stripe
Copy link
Contributor Author

ptal @ob-stripe fixed the $opts['connect_base'] check and added docblocks.

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

LGTM!

@richardm-stripe richardm-stripe merged commit 139ade5 into master Jul 6, 2020
@richardm-stripe richardm-stripe deleted the richardm-add-oauth-service branch July 6, 2020 17:26
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

Successfully merging this pull request may close these issues.

OAuth with client/services pattern
2 participants