Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Merged
merged 2 commits into from

3 participants

@mikegreiling

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.

net/http/Request.php
((22 lines not shown))
- 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 Owner

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 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.

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 Owner

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

@gwoo Owner
gwoo added a note

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

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 Owner
gwoo added a note

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.

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

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

@gwoo
Owner

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

@mikegreiling

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.

@mikegreiling

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.

@nateabele
Owner

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.

@mikegreiling

I have done both. How does it look now?

@nateabele nateabele merged commit ceb0f41 into from
@nateabele
Owner

It looks merge-able. Thanks. :-)

@mikegreiling mikegreiling deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 64 additions and 18 deletions.
  1. +26 −18 net/http/Request.php
  2. +38 −0 tests/cases/net/http/RequestTest.php
View
44 net/http/Request.php
@@ -121,21 +121,20 @@ public function body($data = null, $options = array()) {
*/
public function queryString($params = array(), $format = null) {
$result = array();
+ $query = array();
- foreach (array_filter(array($this->query, $params)) as $query) {
- if (is_string($query)) {
- $result[] = $query;
- continue;
- }
- $query = array_filter($query);
-
- if (!$format) {
- $result[] = http_build_query($query);
+ foreach (array_filter(array($this->query, $params)) as $querySet) {
+ if (is_string($querySet)) {
+ $result[] = $querySet;
continue;
}
+ $query = array_merge($query, $querySet);
+ }
+ $query = array_filter($query);
+
+ if ($format) {
$q = null;
-
- foreach ($params as $key => $value) {
+ foreach ($query as $key => $value) {
if (!is_array($value)) {
$q .= String::insert($format, array(
'key' => urlencode($key),
@@ -151,7 +150,10 @@ public function queryString($params = array(), $format = null) {
}
}
$result[] = substr($q, 0, -1);
+ } else {
+ $result[] = http_build_query($query);
}
+
$result = array_filter($result);
return $result ? "?" . join("&", $result) : null;
}
@@ -172,11 +174,9 @@ public function queryString($params = array(), $format = null) {
* - `'method'` _string_: If applicable, the HTTP method to use in the request.
* Mainly applies to the `'context'` format.
* - `'host'` _string_: The host name the request is pointing at.
- * - `'port'` _string_: The host port, if any. If specified, should be prefixed
- * with `':'`.
+ * - `'port'` _string_: The host port, if any.
* - `'path'` _string_: The URL path.
- * - `'query'` _mixed_: The query string of the URL as a string or array. If passed
- * as a string, should be prefixed with `'?'`.
+ * - `'query'` _mixed_: The query string of the URL as a string or array.
* - `'auth'` _string_: Authentication information. See the constructor for
* details.
* - `'content'` _string_: The body of the request.
@@ -189,7 +189,7 @@ public function to($format, array $options = array()) {
'method' => $this->method,
'scheme' => $this->scheme,
'host' => $this->host,
- 'port' => $this->port ? ":{$this->port}" : '',
+ 'port' => $this->port,
'path' => $this->path,
'query' => null,
'auth' => $this->auth,
@@ -205,6 +205,14 @@ public function to($format, array $options = array()) {
);
$options += $defaults;
+ if (is_string($options['query'])) {
+ $options['query'] = "?" . $options['query'];
+ } else if ($options['query']) {
+ $options['query'] = "?" . http_build_query($options['query']);
+ } else if ($options['query'] === null) {
+ $options['query'] = $this->queryString();
+ }
+
if ($options['auth']) {
$data = array();
@@ -221,7 +229,7 @@ public function to($format, array $options = array()) {
switch ($format) {
case 'url':
- $options['query'] = $this->queryString($options['query']);
+ $options['port'] = $options['port'] ? ":{$options['port']}" : '';
$options['path'] = str_replace('//', '/', $options['path']);
return String::insert("{:scheme}://{:host}{:port}{:path}{:query}", $options);
case 'context':
@@ -237,7 +245,7 @@ public function to($format, array $options = array()) {
);
return array('http' => array_diff_key($options, $defaults) + $base);
case 'string':
- $path = str_replace('//', '/', $this->path) . $this->queryString($options['query']);
+ $path = str_replace('//', '/', $this->path) . $options['query'];
$status = "{$this->method} {$path} {$this->protocol}";
return join("\r\n", array($status, join("\r\n", $this->headers()), "", $body));
default:
View
38 tests/cases/net/http/RequestTest.php
@@ -111,6 +111,17 @@ public function testQueryStringSetup() {
$this->assertEqual($expected, $result);
}
+ public function testQueryStringMerge() {
+ $expected = "?param=foo";
+ $this->request->query = array('param' => 'value');
+ $result = $this->request->queryString(array('param' => 'foo'));
+ $this->assertEqual($expected, $result);
+
+ $expected = "?param=foo&param2=bar";
+ $result = $this->request->queryString(array('param' => 'foo', 'param2' => 'bar'));
+ $this->assertEqual($expected, $result);
+ }
+
public function testToString() {
$expected = join("\r\n", array(
'GET / HTTP/1.1',
@@ -237,6 +248,25 @@ public function testToUrl() {
$this->assertEqual($expected, $result);
}
+ public function testToUrlOverride() {
+ $request = new Request(array(
+ 'scheme' => 'http',
+ 'host' => 'localhost',
+ 'port' => 80,
+ 'query' => array('foo' => 'bar', 'bin' => 'baz')
+ ));
+
+ $result = $request->to('url', array(
+ 'scheme' => 'https',
+ 'host' => 'lithium.com',
+ 'port' => 443,
+ 'query' => array('foo' => 'you')
+ ));
+ $expected = 'https://lithium.com:443/?foo=you';
+
+ $this->assertEqual($expected, $result);
+ }
+
public function testToContext() {
$expected = array('http' => array(
'method' => 'GET',
@@ -306,6 +336,14 @@ public function testParseUrlToConfig() {
$expected = $url;
$result = $request->to('url');
$this->assertEqual($expected, $result);
+
+ $url = "http://localhost:80/path/one.php?param=1&param=2";
+ $config = parse_url($url);
+ $request = new Request($config);
+
+ $expected = $url;
+ $result = $request->to('url');
+ $this->assertEqual($expected, $result);
}
public function testQueryParamsConstructed() {
Something went wrong with that request. Please try again.