Skip to content

Allow full paths in REST requests#136

Merged
paulomarg merged 1 commit intomainfrom
paulo/allow_rest_path_overrides
Feb 17, 2022
Merged

Allow full paths in REST requests#136
paulomarg merged 1 commit intomainfrom
paulo/allow_rest_path_overrides

Conversation

@paulomarg
Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

Currently, we can't handle requests like access scopes (/admin/oauth/access_scopes.json) or token access (/admin/api_permissions/current.json), because these resources use custom paths instead of the default /admin/api/{version}/....

This means we can't easily reach those endpoints unless the code uses the base HTTP client that the REST one is built upon, which makes things unnecessarily complicated.

WHAT is this pull request doing?

This PR solves that problem by allowing REST calls in the Admin API client to be made using a full path. Therefore, requests like

$client = $client = new Rest($this->domain, 'token');
$response = $client->get('/admin/oauth/access_scopes.json');

work just like any regular request would.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above
  • I have added/updated tests for this change
  • I have updated the documentation for public APIs from the library (if applicable)

@paulomarg paulomarg requested a review from a team as a code owner February 10, 2022 22:03
@paulomarg paulomarg force-pushed the paulo/allow_rest_path_overrides branch from 044893d to 54e20d2 Compare February 10, 2022 22:04
Comment thread src/Clients/Rest.php Outdated
Comment thread src/Clients/Rest.php
Comment on lines +66 to +67
if (strpos($path, "/admin") !== 0) {
$path = "/admin/api/$apiVersion$path";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

instead of assuming $path has a / with the getRequestPath helper, can we instead:

Suggested change
if (strpos($path, "/admin") !== 0) {
$path = "/admin/api/$apiVersion$path";
if ($path[0] != '/') {
$path = "/admin/api/$apiVersion/$path";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

parent::getRequestPath is set up to always ensure the path has a / at position 0, which of course we could change here, but I wasn't too keen on using just the slash as a marker for a full path because I felt it would be too error prone.

My main concern was that folks would try things like /product.json, which I think should just be prefixed with the default.

@paulomarg paulomarg force-pushed the paulo/allow_rest_path_overrides branch from 54e20d2 to b07692d Compare February 16, 2022 19:27
@paulomarg paulomarg requested a review from teddyhwang February 16, 2022 19:28
@paulomarg paulomarg force-pushed the paulo/allow_rest_path_overrides branch from b07692d to 5b42cee Compare February 16, 2022 19:41
@paulomarg paulomarg merged commit 3d9c5c6 into main Feb 17, 2022
@paulomarg paulomarg deleted the paulo/allow_rest_path_overrides branch February 17, 2022 15: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.

2 participants