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

Fixed inline/attachment issue. Added new tests. #151

Closed
wants to merge 1 commit into from

Conversation

travelton
Copy link
Contributor

Hello Friends!

This pull request fixes a multi inline/attachment issue. Some HTTP clients cannot handle url encoded form data with the same key and different value. With the current implementation, if you pass in multiple inline/attachment values, only the last value in the array is actually sent to the Mailgun API.

We need to give the key an "index marker" (for lack of better term?). The Mailgun API knows how to handle this...

Example (Bad):

inline=blah&inline=blah

Example (Good):

inline[0]=blah&inline[1]=blah

Also, tests. I'm including my lame attempt at revamping the tests a bit. Eventually, every test should probably follow something like this, so as to test between request and "$client->send()". Happy to entertain any community suggestions here. We really need to beef up the tests on this SDK.

*
* @return array
*/
protected function prepareFile($fieldName, $filePath)
protected function prepareFile($fieldName, $filePath, $fileIndex=0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Code style $fileIndex = 0

@Nyholm
Copy link
Collaborator

Nyholm commented Aug 5, 2016

Thank you @travelton. This is a great PR.

Im not happy with how you write the tests. I think that is a hack. But your point is valid. I'll send a PR shortly that achieve the same goals with the tests. Give me a moment and I'll have something to show

@Nyholm Nyholm mentioned this pull request Aug 5, 2016
@Nyholm
Copy link
Collaborator

Nyholm commented Aug 5, 2016

See #153.
I was planning to do something like the code example in the PR but doing that would be way to much boilerplate code for each test.

@travelton
Copy link
Contributor Author

@Nyholm Agreed, totally hacky. Removing my tests. Once you have your tests merged, I'll rebase this PR and write some tests for it.

@Nyholm
Copy link
Collaborator

Nyholm commented Aug 5, 2016

Awesome. Im glad you liked it. I've merged the #153 now

@Nyholm Nyholm mentioned this pull request Aug 10, 2016
Nyholm added a commit that referenced this pull request Aug 10, 2016
@Nyholm
Copy link
Collaborator

Nyholm commented Aug 10, 2016

Merged by #156

@Nyholm Nyholm closed this Aug 10, 2016
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