This repository has been archived by the owner on Jun 27, 2022. It is now read-only.
Fix OauthPlugin query aggregation always being default #15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| ->set('g', array('h', 'i', 'j')) | ||
| ->set('k', array('l')) | ||
| ->set('m', array('n', 'o')); | ||
| // print_r($request->getQuery()); die; |
There was a problem hiding this comment.
Can you remove this line please?
There was a problem hiding this comment.
Eep, sorry for missing this. Fixed in 562546b.
|
Nice work. I don't find this dirty at all. I'll merge after the debug line is removed the tests. |
The Oauth "string to sign" includes the query string parameters, together with the OAuth parameters. Guzzle uses a QueryString instance to combine the two. However, it was incorrectly using the default aggregator, instead of the actual aggregator used by the QueryString that's part of the request. This change uses the QueryString from the request to serialise the parameters to be signed. This will affect the aggregation of the Oauth parameters as well -- however aggregation should never occur for these because there is only one value for each key.
|
Thanks @mtdowling. Fixed the issue as requested and tests are passing. |
|
Thanks! |
mtdowling
added a commit
that referenced
this pull request
Mar 19, 2014
Fix OauthPlugin query aggregation always being default
|
Will this make it into a tagged release? I'm currently having to depend on |
|
Yeah, but I'm not sure when the next release will happen. Soon I hope. |
|
Thanks. I was just checking that the delay wasn't accidental. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the same as guzzle/guzzle#596, but submitted to the correct repository for Guzzle 3. Here's what I wrote there describing the issue:
I took @mtdowling's advice, and this time avoided a
QueryString::getAggregator()method. Instead I clone the entireQueryStringfrom the request, then replace its data with the parameters to be signed.It's a bit dirty as it unnecessarily subjects the
oauth_*parameters to the query aggregation strategy of the request'sQueryString. However, there should be no effects of this -- because these never have arrays as their value, they will never be subjected to aggregation anyway. If this approach is also undesirable, I'll probably need to hand the implementation over to a maintainer as I don't think I'll have the time to do a more significant refactoring.