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

Fix serialization of query params #129

Merged
merged 1 commit into from
Jan 13, 2017

Conversation

bakura10
Copy link
Contributor

@bakura10 bakura10 commented Jan 10, 2017

Addresses #128

This PR is likely a BC of 1, but as the behaviour of 1.0.0 is wrong and completely different from previous version, I think it's more a bug fix rather than a BC. Also there was no test to verify this behaviour.

Currently, the Guzzle Service serializes incorrectly any query params that are array or object.

For instance, considering this operation's param:

'foo' => [
  'type' => 'array',
  'location' => 'query'
]

Specifying this parameter: $client->myOperation(['foo' => [1, 2]]) would serialize this query parameter : foo=1&foo=2, which is completely different from the older behaviour and, from my understanding, incorrect.

Similarily, when specifying an object type, this: $client->myOperation(['foo' => ['bar' => 'baz']]) would serialize this query "foo=baz" (hence completely loosing the context of bar).

After this PR, first operation would be serialized 'foo[0]=1&foo[1]=2' and second operation would be serialized 'foo[bar]=baz'.

Thanks a lot!

@Konafets Konafets changed the base branch from master to develop January 13, 2017 10:00
@Konafets
Copy link
Contributor

Thank you very much for your contribution. You fix looks good and you even add tests. Thumbs up!

@Konafets Konafets merged commit 7846618 into guzzle:develop Jan 13, 2017
@bakura10 bakura10 deleted the fix-query-serialization branch January 13, 2017 10:03
@bakura10
Copy link
Contributor Author

Thanks a lot ! It seems there is a lot of missing coverage with serializing query attributes, as I'm maintaining some big descriptors and starting to upgrade them, it's possible that I encounter other regressions. I'll make sure to fix them as they arise :).

In the mean time would it be possible to tag this ? :)

@bakura10
Copy link
Contributor Author

Hey !

Apparently I've messed and I've made the PR against the "develop" branch. Should I re-do the PR to master so that this can be tagged?

@Konafets
Copy link
Contributor

Konafets commented Jan 13, 2017

Nope. I changed it to develop intentionally because master only should contain tagged versions. I follow git flow here: http://danielkummer.github.io/git-flow-cheatsheet/

@Konafets Konafets self-assigned this Jan 13, 2017
@Konafets Konafets added this to the 1.0.1 milestone Jan 13, 2017
@Konafets
Copy link
Contributor

Konafets commented Jan 13, 2017

@bakura10 Just tagged version 1.0.1. Hope it works for now. You are very welcome to open issues and/or PRs. I will try to get everything fixed as soon as possible.

@Konafets Konafets added the bug label Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants