Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

minor update to Request::to() and Request::queryString() #854

Merged
merged 2 commits into from Mar 17, 2013

Conversation

Projects
None yet
3 participants
Contributor

mikegreiling commented Mar 8, 2013

Request::to() allows query strings to be overridden rather than appended.
Request::queryString() allows query parameters to be merged rather than appended.

also removed arbitrary requirement that $port must be pre-formatted with a colon in the parameters.

@nateabele nateabele and 1 other commented on an outdated diff Mar 8, 2013

net/http/Request.php
- foreach ($params as $key => $value) {
- if (!is_array($value)) {
- $q .= String::insert($format, array(
- 'key' => urlencode($key),
- 'value' => urlencode($value)
- ));
- continue;
- }
- foreach ($value as $val) {
- $q .= String::insert($format, array(
- 'key' => urlencode("{$key}[]"),
- 'value' => urlencode($val)
- ));
+ $query = array_merge($query, $querySet);
+ }
+ if ($query = array_filter($query)) {
@nateabele

nateabele Mar 8, 2013

Owner

These control structures are too deeply nested. This should be refactored.

@mikegreiling

mikegreiling Mar 8, 2013

Contributor

I'm actually not too fond of the way this method came out... The original premise of this method is a bit messy and not particularly useful I think. A lot of care is taken to retain and sloppily combine both the $params and the Request object's $query property, and the tests are all written around this expectation. I think it would be more useful to simply have one override the other and not worry about merging. The merging could be done by the caller of the function prior to passing in the array of parameters in cases where retaining and amending the original query is important.

That would, however, be a BC break and I'd have to rewrite the tests. Thoughts?

If not, I think I'll just remove this part of the commit.

@nateabele nateabele and 2 others commented on an outdated diff Mar 8, 2013

tests/cases/net/http/RequestTest.php
@@ -299,7 +329,7 @@ public function testDigest() {
}
public function testParseUrlToConfig() {
- $url = "http://localhost/path/one.php?param=1&param=2";
+ $url = "http://localhost:80/path/one.php?param=1&param=2";
@nateabele

nateabele Mar 8, 2013

Owner

If we're going to set it up where the port just shows up even if unspecified, then we need to include a mapping where the port can be omitted if it's the default for the protocol.

@mikegreiling

mikegreiling Mar 8, 2013

Contributor

The port won't show up if unspecified. The way this test works is it disassembles a url with parse_url() and recompiles it using Request::to('url'). I merely added a port to the string to demonstrate that it, too will follow through. Prior to the changes in Request::to(), however, this assertion would have failed.

@nateabele

nateabele Mar 8, 2013

Owner

Ah, I misread it then. Carry on. :-)

@gwoo

gwoo Mar 8, 2013

Owner

Adding a new test is preferred to changing an old test.

@mikegreiling

mikegreiling Mar 8, 2013

Contributor

True, though this was such a minor tweak I didn't think a new test was strictly necessary. All it did was slightly expand the scope of the original test to encompass the port number.

@gwoo

gwoo Mar 8, 2013

Owner

Changing a test, whether slightly or by a lot will indicate a change in functionality. Without the previous functionality documented by another test, you end up with the confusion of Nate.

Owner

gwoo commented Mar 8, 2013

Is the intent of this PR to have the two methods do two different things?

Owner

gwoo commented Mar 8, 2013

The original into was for Request::to() to only rely on Request::queryString()

Contributor

mikegreiling commented Mar 8, 2013

I suppose this does change the functionality a bit, however, not in any way that broke any existing tests.

I think implementing Request::to() in this way is more consistant with the documentation in the comment block preceding the function which states that the 'query' parameter can be overridden, when in fact it cannot. What it did before was concatenate what was provided with the existing query data, and did so poorly in my opinion, because it wouldn't even recognize and remove duplicate query parameters.

Contributor

mikegreiling commented Mar 8, 2013

I originally set out to merely update queryString() to more intelligently combine sets of queries, but then more generally thought it would be nice to be able to override the query string entirely within the to('url') method to be more consistent with the way other attributes of the request are overridden (like host, port, scheme, etc.), so I tried to find a way to do so without breaking any current tests.

Ideally, as I described in the comment under queryString(). I'd like to change it's implementation from merging to replacing, and then use that within the to() method as before, but that would be a BC break that would require rewriting some tests...

Either way, I don't think I care enough about this to strongly debate the point. I can withdraw my PR if you guys think it's ill-advised.

Owner

nateabele commented Mar 8, 2013

I don't really think it's worth having BC-breaking changes here, but the feature you're proposing seems like a good idea. If you can (a) copy the test, and (b) refactor the deeply-nested control structures, I'll get it merged.

Contributor

mikegreiling commented Mar 17, 2013

I have done both. How does it look now?

@nateabele nateabele added a commit that referenced this pull request Mar 17, 2013

@nateabele nateabele Merge pull request #854 from pixelcog/request-to
minor update to Request::to() and Request::queryString()
ceb0f41

@nateabele nateabele merged commit ceb0f41 into UnionOfRAD:dev Mar 17, 2013

1 check passed

default The Travis build passed
Details
Owner

nateabele commented Mar 17, 2013

It looks merge-able. Thanks. :-)

@mikegreiling mikegreiling deleted the pixelcog:request-to branch Mar 17, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment