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

Adds the query parameter to the guzzle call for GET requests. #5

Merged
merged 1 commit into from Sep 21, 2017

Conversation

typhonius
Copy link
Contributor

Without this, any GET request is not able to pass optional parameters. The following snippet, for example, would continue to use default parameters without this patch.

$dns->listRecords($zoneID', '', '', 2, 40);

@IcyApril
Copy link
Contributor

Thanks for suggesting this @typhonius; happy to merge, but there are a few bits of pre-requisite work first:

  • Most importantly the Adapter interface itself needs to match the updated Guzzle class
  • Secondly, in order to pass these parameters from the Endpoints classes, we are using http_build_query, if we are to use this method - those classes need to be updated too.

Thanks!

@typhonius typhonius force-pushed the add-query-to-get branch 3 times, most recently from bc35c87 to 9c65833 Compare September 21, 2017 00:15
@typhonius
Copy link
Contributor Author

Thanks @IcyApril, I've updated the patch to cover all GET requests made by this package. I've also updated tests to meet the expected format.

@typhonius
Copy link
Contributor Author

typhonius commented Sep 21, 2017

Re-looking at this today with the latest master I'm not running into the issue anymore so we may be able to close this out.

@IcyApril IcyApril merged commit b91dd5e into cloudflare:master Sep 21, 2017
@IcyApril
Copy link
Contributor

Perfect, thanks for the work on this. I'll merge this to go into the next release.

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.

None yet

2 participants