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.

Sign up for GitHub

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

Jump to bottom

Add return request object #278

Merged
merged 4 commits into from
Aug 15, 2018
Merged

Add return request object #278

merged 4 commits into from
Aug 15, 2018

Conversation

joshcanhelp
Copy link
Contributor

@joshcanhelp joshcanhelp commented Aug 7, 2018 •
edited
Loading

This PR adds the ability to define a return type other than the data retrieved from the API. Return types supported match what Guzzle can return:

  • body - Existing installs that do not define this type will default to body, which is an array from json_decode. This preserves backwards-compat.
  • headers - Just the headers returned by the API call.
  • object - The complete Guzzle Response object

Example:

$mgmt_api = new Management(
    getenv('AUTH0_MANAGEMENT_API_TOKEN'), 
    $a0_domain, 
    [],
    'object' // New parameter
);

$guzzle_response = $mgmt_api->clients->getAll();

foreach ($guzzle_response->getHeaders() as $header => $values) {
    echo '<li><strong>' . $header . '</strong>: ' . $values[0] . '</li>';
}

Specific changes:

  • Add returnType protected var to ApiClient class
  • Add returnType protected var to RequestBuilder class along with returnTypes to define valid values.
  • Add additional code commenting.

Closes #277.

@joshcanhelp joshcanhelp added this to the v5-Next milestone Aug 7, 2018
joshcanhelp
joshcanhelp commented Aug 7, 2018
View reviewed changes
tests/API/Management/AuthApiDBConnectionsTest.php Outdated
@@ -25,7 +25,7 @@ public function testSignup()
$api = new Authentication($env['DOMAIN'], $env['APP_CLIENT_ID']);

$email = $this->email;
$password = '123-xxx-23A-bar';
$password = 'Bqn8LEsu68p38TmFvsWW';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Password was not passing the security policy.

joshcanhelp
joshcanhelp commented Aug 7, 2018
View reviewed changes
tests/API/Management/AuthApiTest.php
@@ -46,7 +51,7 @@ public function testAuthorizeWithRO()

$this->assertArrayHasKey('email', $userinfo);
$this->assertArrayHasKey('email_verified', $userinfo);
$this->assertArrayHasKey('user_id', $userinfo);
$this->assertArrayHasKey('sub', $userinfo);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sub is a better key to check, will exist whether using Legacy or not.

@joshcanhelp joshcanhelp force-pushed the add-return-request-object branch from fa88d71 to 1efe8a3 Compare August 7, 2018 16:05
cocojoe
cocojoe reviewed Aug 9, 2018
View reviewed changes
Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

Why add so much vs saying just returning the response headers, which is what was asked for?

Does the developer have to make multiple requests to retrieve different pieces of info?

src/API/Helpers/RequestBuilder.php
@@ -1,101 +1,133 @@
<?php
/**
Copy link
Member

Choose a reason for hiding this comment

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

When you lowkey removing German 😆

src/API/Helpers/RequestBuilder.php Outdated
*
* @var array
*/
protected $returnTypes = [ 'body', 'headers', 'object', 'protocolVersion', 'reasonPhrase', 'statusCode' ];
Copy link
Member

Choose a reason for hiding this comment

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

Why so many options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything that Guzzle can return, might as well include them since the library is capable.

src/API/Helpers/RequestBuilder.php Outdated

case 'body':
default:
$body = (string) $response->getBody();
Copy link
Member

Choose a reason for hiding this comment

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

when it is not json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure but no harm in checking

src/API/Helpers/RequestBuilder.php
}

if (! in_array($type, $this->returnTypes)) {
throw new CoreException('Invalid returnType');
Copy link
Member

Choose a reason for hiding this comment

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

How can this be thrown? It looks like type if missing is default to body in the first check. So it will then pass the in_array check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could pass in banana and it wouldn't know what to return.

@joshcanhelp joshcanhelp force-pushed the add-return-request-object branch from 1efe8a3 to 6107947 Compare August 9, 2018 15:25
@lbalmaceda
Copy link
Contributor

@joshcanhelp can you please add a snippet showing how a single request would be done after this changes, and how all those "return types" are being accessible now?

@joshcanhelp
Copy link
Contributor Author

@cocojoe - Removed everything except headers, body, and object return types.

@lbalmaceda - Added an example 👍

@cocojoe
Copy link
Member

cocojoe commented Aug 15, 2018

Great to see an example in description, we should use that more 👍

cocojoe
cocojoe approved these changes Aug 15, 2018
View reviewed changes
Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

LGTM

@joshcanhelp joshcanhelp merged commit 5af338b into master Aug 15, 2018
@joshcanhelp joshcanhelp deleted the add-return-request-object branch August 15, 2018 14:21
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cocojoe cocojoe cocojoe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@joshcanhelp @lbalmaceda @cocojoe