Adding pagination, tests, + docs to Client Grants; minor test suite refactor #271
Conversation
| */ | ||
| public function get($id, $audience = null) | ||
| public function getAll(array $params = [], $page = null, $per_page = null) |
There was a problem hiding this comment.
Looks like I'm changing the method name here but I'm not. The get method still exists (in a semi-broken state), moved to the bottom of the class with a TODO to deprecate.
joshcanhelp
force-pushed
the
add-pagination-client-grants
branch
from
2013966 to
34e5d12
Compare
| */ | ||
| public function getByAudience($audience, $page = null, $per_page = null) | ||
| { | ||
| if (empty($audience) || ! is_string($audience)) { |
There was a problem hiding this comment.
So the rest of the code follows this, although I think it's ugly. If it's consistent with the rest of the code base, then I'm fine with it.
There was a problem hiding this comment.
"Ugly" as in you don't like the error checking here? @lbalmaceda and I talked about failing early in methods to avoid API errors. For this one, an empty $audience would not fail but it also would not filter so the method would be useless.
There was a problem hiding this comment.
Don't like the space between a ! and the context. But don't have any strong feelings as all the others are like this in the PR so presume it's inline with coding style of the SDK.
There was a problem hiding this comment.
I was on the fence about the space. This is all whitespace-corrected now so I can enforce no space.
| * | ||
| * @link https://auth0.com/docs/api/management/v2#!/Client_Grants/get_client_grants | ||
| */ | ||
| public function getByClientId($client_id, $page = null, $per_page = null) |
There was a problem hiding this comment.
Is this convenience method also in other SDKs?
/cc @lbalmaceda
There was a problem hiding this comment.
Not that I know of but I don't think that's a good reason not to add it here. Make the developer's life easier!
There was a problem hiding this comment.
Just curious, it's always good to create more convenience for the end user, balanced with maintainability.
| * | ||
| * @link https://auth0.com/docs/api/management/v2#!/Client_Grants/get_client_grants | ||
| */ | ||
| public function getByAudience($audience, $page = null, $per_page = null) |
There was a problem hiding this comment.
Is this convenience method also in other SDKs?
/cc @lbalmaceda
| * @return mixed | ||
| * | ||
| * @throws \Exception Thrown by the Guzzle HTTP client when there is a problem with the API call. |
There was a problem hiding this comment.
Although we use Guzzle I would not mention it by name, goes for all mentions in this PR.
There was a problem hiding this comment.
We use it in the variable name for the API. It will also appear in the error name. I think it's helpful here in case someone is getting a fatal error and want to know where to start troubleshooting.
| */ | ||
| public function create($client_id, $audience, $scope) | ||
| public function delete($id) |
There was a problem hiding this comment.
So presume audience is no longer required? What happens in PHP if a developer makes a call to a method that has a change in signature?
There was a problem hiding this comment.
Bad diff for this one ... you can see this is a different method.
To answer your question though ... it wouldn't fail, it just wouldn't use that param.
|
@joshcanhelp in the description please list the Public API changes, list of method/signatures added, changed etc |
| */ | ||
| public function create($client_id, $audience, $scope) | ||
| public function delete($id) |
There was a problem hiding this comment.
Indeed, the one I saw was delete($id) vs delete($id, $audience = null)
|
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. |
getAllmethod with pagination parametersgetByAudiencemethod to filter by audiencegetByClientIdmethod to filter by client IDCoreExceptionthrows tocreateif required params are empty or invalidgetmethod (cannot get a Client Grant by ID so the method is useless)getApiStatic(will propagate throughout testing code base in different PR)