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

feat: Build URL with multiple instances of the same param #136

Merged
merged 2 commits into from
Mar 18, 2021

Conversation

agh1
Copy link
Contributor

@agh1 agh1 commented Nov 18, 2019

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

A number of APIs accept what is documented as "array of string". In practice, it's a number of instances of identical URL arguments with different values.

For example from retrieving subuser reputations:

https://api.sendgrid.com/v3/subusers/reputations?usernames=myfirstsubuser&usernames=mysecondsubuser&usernames=mythirdsubuser

I updated the buildUrl() function to accept nested arrays so that you could form the above URL with the query:

$queryParams = [
  'usernames' => [
    'myfirstsubuser',
    'mysecondsubuser',
    'mythirdsubuser',
  ],
];

If you have questions, please send an email to Sendgrid, or file a Github Issue in this repository.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Nov 18, 2019
@SendGridDX
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@agh1
Copy link
Contributor Author

agh1 commented Nov 18, 2019

Regex credit to https://stackoverflow.com/a/8171667

@agh1
Copy link
Contributor Author

agh1 commented Nov 18, 2019

@thinkingserious per sendgrid/sendgrid-php#828 is the CLA still a thing?

@thinkingserious thinkingserious changed the base branch from master to main July 28, 2020 14:38
@thinkingserious
Copy link
Contributor

Since there has been no activity on this issue since March 1, 2020, we are closing this issue. If you are still interested in moving this PR forward, please feel free to reopen. Thank you!

@agh1
Copy link
Contributor Author

agh1 commented Mar 11, 2021

@thinkingserious I was waiting on a response to this pull request, and I'm happy to do what's needed to move it forward if you have any feedback.

I would like to reopen it, but I don't appear to have the permission to do so.

Copy link
Contributor

@shwetha-manvinkurke shwetha-manvinkurke left a comment

Choose a reason for hiding this comment

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

LGTM! Just gotta merge changes with main

lib/Client.php Outdated Show resolved Hide resolved
lib/Client.php Show resolved Hide resolved
@shwetha-manvinkurke shwetha-manvinkurke changed the title Build URL with multiple instances of the same param feat: Build URL with multiple instances of the same param Mar 15, 2021
@agh1
Copy link
Contributor Author

agh1 commented Mar 17, 2021

@shwetha-manvinkurke thanks for the review! I fixed the leading space in the doc block and rebased against main.

Copy link
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

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

Thank you!

@thinkingserious thinkingserious merged commit 3fbaf42 into sendgrid:main Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants