Skip to content

Improve Li3 Request/Response Classes #902

Merged
merged 6 commits into from Apr 23, 2013

4 participants

@mikegreiling

There's quite a few changes in this PR, so I'll go through them one at a time:

1) Added Cookie header compiling to \net\http\Request.

You can now do stuff like this:

$request = new Request(array(
    'host' => 'lithify.me',
    'path' => '/test',
    'cookies' => array(
        'foo' => 'bar',
        'bin' => 'baz'
    )
));

$curl = new Curl();
$curl->open();
$curl->write($request);

and it will send the cookies along to the server. This is useful for maintaining session ids or other stateful data between multiple remote server requests/responses.

2. Refactored \net\http\Message::header() and allow for multiple headers of the same name when parsing a Response message.

The header() method now has an option to specify whether headers are to be appended or overwritten when being set. Appending is now the default action when reading headers through \net\http\Response::_parseMessage().

$message = join("\r\n", array(
    'HTTP/1.1 200 OK',
    'Connection: close',
    'Content-Type: text/plain;charset=UTF8',
    'Header: value1',
    'Header: value2',
    '',
    'Test!'
));
$header = array('value1', 'value2');

$response = new Response(compact('message'));
$this->assertEqual($header, $response->headers('Header'));
$this->assertEqual($message, (string) $response);

The header method now also calls itself recursively when setting a $key => $value pair so it will be far more useful to any subclass which overloads it.

3. Added Set-Cookie header parsing and compiling to \net\http\Response

Cookies in response messages are now parsed from:

Set-Cookie: Name=Marty%20McFly; Domain=.hillvalley.us; Secure
Set-Cookie: Destination=The%20Future; Expires=Wed, 21-Oct-2015 23:29:00 GMT

Into an associative array of the form:

array(
    'Name' => array('value' => 'Marty McFly', 'domain' => '.hillvalley.us', 'secure' => true),
    'Destination' => array('value' => 'The Future', 'expires' => 'Oct 21 2015 4:29 PM PDT')
)

4. Made \action\Request::header() and to('string') useful again.

You can now call $request->header('Foo-Bar') to access any arbitrary header without the far less intuitive $request->env('HTTP_FOO_BAR') that it gets translated to in the webserver environment.

The entire request (sans attachments) can be serialized with $request->to('string') consistant with its parent class. This can be useful when debugging if you want to capture an approximation of the actual http request message for your logs along with a stack trace when an exception occurs.

Also moved request data parsing within \action\Request::_init() back into body() to be consistant with parent classes.

@jails jails commented on the diff Apr 23, 2013
action/Request.php
@@ -565,6 +568,39 @@ public function type($type = null) {
}
/**
+ * Expands on `\net\http\Message::headers()` by translating field names and values to those
+ * provided by the server environment.
+ *
+ * @param string $key
+ * @param string $value
+ * @param boolean $replace
+ * @return mixed
+ */
+ public function headers($key = null, $value = null, $replace = true) {
+ if (is_string($key) && !isset($this->headers[$key]) && $value === null) {
+ $env = strtoupper(str_replace('-', '_', $key));
+ if (!in_array($env, array('CONTENT_TYPE', 'CONTENT_LENGTH'))) {
@jails
Union of RAD member
jails added a note Apr 23, 2013

Imo initialization of env vars should be moved to env() (in the big switch case)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jails jails commented on the diff Apr 23, 2013
action/Request.php
@@ -565,6 +568,39 @@ public function type($type = null) {
}
/**
+ * Expands on `\net\http\Message::headers()` by translating field names and values to those
+ * provided by the server environment.
+ *
+ * @param string $key
+ * @param string $value
+ * @param boolean $replace
+ * @return mixed
+ */
+ public function headers($key = null, $value = null, $replace = true) {
+ if (is_string($key) && !isset($this->headers[$key]) && $value === null) {
+ $env = strtoupper(str_replace('-', '_', $key));
+ if (!in_array($env, array('CONTENT_TYPE', 'CONTENT_LENGTH'))) {
+ $env = 'HTTP_' . $env;
+ }
+ if (!empty($this->_env[$env])) {
@jails
Union of RAD member
jails added a note Apr 23, 2013

It may be better to use $this->env() instead of $this->_env ?

@al-the-x
al-the-x added a note Apr 23, 2013

👍 Accessors are there for a reason, even inside of the providing class...

@mikegreiling
mikegreiling added a note Apr 23, 2013

see comment here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jails jails commented on the diff Apr 23, 2013
action/Request.php
+ * @param boolean $replace
+ * @return mixed
+ */
+ public function headers($key = null, $value = null, $replace = true) {
+ if (is_string($key) && !isset($this->headers[$key]) && $value === null) {
+ $env = strtoupper(str_replace('-', '_', $key));
+ if (!in_array($env, array('CONTENT_TYPE', 'CONTENT_LENGTH'))) {
+ $env = 'HTTP_' . $env;
+ }
+ if (!empty($this->_env[$env])) {
+ $this->headers($key, $this->_env[$env]);
+ return $this->_env[$env];
+ }
+ }
+ if (!$key) {
+ foreach($this->_env as $name => $value) {
@jails
Union of RAD member
jails added a note Apr 23, 2013

env variables are lazily initialized so all HTTP_* won't exists if $this->env() is not explicitely called in some configuration ?
Is a exhausive list can't be provided here ?

@mikegreiling
mikegreiling added a note Apr 23, 2013

On the contrary, $this->_env is initialized with $this->_config['env'] ($_ENV + $_SERVER when globals is true) in the __construct() method.

All HTTP headers apart from Content-Type and Content-Length (at least in most server configurations of nginx and apache) end up in $_SERVER as $_SERVER['HTTP_HEADER_NAME'] so a call utilizing $this->env() magic isn't necessary. We just want the raw input from the environment variables.

See the comments here: http://www.php.net/manual/en/function.getallheaders.php#104307

@jails
Union of RAD member
jails added a note Apr 23, 2013

Yeah thought about theses specific values. Since CONTENT_TYPE seems to be the correct naming over HTTP_CONTENT_TYPE.

Does:

public function headers($key = null, $value = null, $replace = true) {
    if (!$key) {
        foreach($this->_env as $name => $value) {
            if ($name === 'HTTP_CONTENT_TYPE' || $name === 'HTTP_CONTENT_LENGTH') {
                continue;
            }
            if (($tmp = substr($name, 0, 5)) === 'HTTP_') {
                $name = str_replace(' ', '-', ucwords(strtolower(str_replace('_', ' ', $tmp))));
                $this->headers($name, $this->env($name));
            }
        }
        $this->headers('Content-Type', $this->env('CONTENT_TYPE'));
        $this->headers('Content-Length', $this->env('CONTENT_LENGTH'));
    }
    return parent::headers($key, $value, $replace);
}

And let 'CONTENT_TYPE' / 'CONTENT_LENGTH' be initialized by the ::env() function make sense for you ?

a kind of:

    case 'HTTP_CONTENT_TYPE':
        $val = 'text/html';
    break;
    case 'CONTENT_TYPE':
        return $this->env('HTTP_CONTENT_TYPE');
    break;
    case 'CONTENT_LENGTH':
        return $this->env('HTTP_CONTENT_LENGTH');
    break;
@mikegreiling
mikegreiling added a note Apr 23, 2013

All headers() is doing is translating a call to $request->headers('Content-Type') into $_SERVER['CONTENT_TYPE'] and calls to $response->headers('Arbitrary-Header') into $_SERVER['HTTP_ARBITRARY_HEADER'].

It could utilize $this->env(), but if you're looking to retrieve the actual headers received in the request via $request->headers() I think you'd want the unadulterated response from $_SERVER rather than using any magic from $this->env() to fill in defaults.

A call to $this->env('HTTP_HOST') will return a default value even if there was no Host header sent in the request. I think this is what differentiates header() from being a mere proxy for env().

But, I'm open to more input on it, if you guys think that's a confusing policy.

I mainly implemented headers() so that I can do a $request->to('string') for some of my debugging and retrieve a close approximation of the actual request received by the web host.

@jails
Union of RAD member
jails added a note Apr 23, 2013

makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jails jails and 2 others commented on an outdated diff Apr 23, 2013
net/http/Response.php
+ $this->cookies = array();
+ }
+ if (is_array($key)) {
+ foreach ($key as $cookie => $value) {
+ $this->cookies($cookie, $value, $expire, $path, $domain, $secure, $httponly);
+ }
+ } elseif (is_string($key)) {
+ if ($value === null) {
+ return isset($this->cookies[$key]) ? $this->cookies[$key] : null;
+ }
+ if ($value === false) {
+ unset($this->cookies[$key]);
+ } else {
+ if (is_array($value)) {
+ if (array_values($value) === $value) {
+ foreach ($value as &$set) {
@jails
Union of RAD member
jails added a note Apr 23, 2013

I noticed the $data as &$value syntax perfoms extremely slowly in some configuration (even with a PHP 5.4). Imo it may be better to use the classic syntax $data as $key => $value.

@mikegreiling
mikegreiling added a note Apr 23, 2013

That's strange, is this true? I can refactor this to use $data as $key => $value as you suggest. Has anyone else run into this oddity?

@nateabele
Union of RAD member
nateabele added a note Apr 23, 2013

Yeah, there's a site somewhere that shows the performance of each different type of loop across various different versions of PHP, and I remember this being one of the slower ones.

@jails
Union of RAD member
jails added a note Apr 23, 2013

The last time I noticed this was on li3_quality. Just changed this line https://github.com/UnionOfRAD/li3_quality/blob/master/analysis/Parser.php#L305 to:

foreach ($tokens as $tokenId => &$token) {

and replacing all $tokens[$tokenId] to $token in the loop.

Previously the syntax check was done in 52s for all the core. After this minor change, I canceled the parsing after 15min of run.

@mikegreiling
mikegreiling added a note Apr 23, 2013
  • updated

perhaps we should look through the rest of the lithium library for uses of $x as &$y because I'm sure this is not the only place...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jails jails commented on the diff Apr 23, 2013
net/http/Response.php
+ if (!$key) {
+ $key = $this->cookies;
+ $this->cookies = array();
+ }
+ if (is_array($key)) {
+ foreach ($key as $cookie => $value) {
+ $this->cookies($cookie, $value, $expire, $path, $domain, $secure, $httponly);
+ }
+ } elseif (is_string($key)) {
+ if ($value === null) {
+ return isset($this->cookies[$key]) ? $this->cookies[$key] : null;
+ }
+ if ($value === false) {
+ unset($this->cookies[$key]);
+ } else {
+ if (is_array($value)) {
@jails
Union of RAD member
jails added a note Apr 23, 2013

Is this if can be simplified with imposing a format convention to $value ?

@mikegreiling
mikegreiling added a note Apr 23, 2013

I'm not quite sure what you're asking here

@jails
Union of RAD member
jails added a note Apr 23, 2013

Looks like $value can be a boolean, a string, an array and an array where array_values($value) === $value. My understanding of HTTP headers is limited. Just wonder if this cascading if can't be simplified by using a convention for $value.

@mikegreiling
mikegreiling added a note Apr 23, 2013

Yeah, it got a bit confusing...

If value is a null or false, it corresponds to "get" or "unset" actions on the key. If it is a string, it is a shorthand which gets transformed into array('value' => $string). If array_values($value) === $value it really means that it's an array of associative arrays, which is necessary to allow for multiple Set-Cookie headers with the same cookie name and different options.

Unfortunately I couldn't think of a more succinct way to do it. I suppose I could replace array_values($value) === $value with isset($value['value']) to be slightly less confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nateabele nateabele and 1 other commented on an outdated diff Apr 23, 2013
net/http/Response.php
+ *
+ * NOTE: Cookie values are stored as an associative array containing at minimum a `value` key.
+ * Cookies which have been set multiple times do not overwrite each other. Rather they are stored
+ * as an array of associative arrays.
+ *
+ * @see http://php.net/manual/en/function.setcookie.php
+ * @param string $key
+ * @param string $value
+ * @param mixed $expire
+ * @param string $path
+ * @param string $domain
+ * @param boolean $secure
+ * @param boolean $httponly
+ * @return mixed
+ */
+ public function cookies($key = null, $value = null, $expire = 0, $path = null, $domain = null, $secure = false, $httponly = false) {
@nateabele
Union of RAD member
nateabele added a note Apr 23, 2013

The method signature for this should really be $key = null, $value = null, array $options = array(), and everything that comes after $value should just be flags inside $options.

@mikegreiling
mikegreiling added a note Apr 23, 2013

I can change this... I just thought it might help to have full parity with setcookie()'s parameters since many people are familiar with it.

Note that cookies('foo', array('value' => 'bar', 'path' => '/', 'domain' => 'google.com')) syntax already achieves the same effect.

@nateabele
Union of RAD member
nateabele added a note Apr 23, 2013

Heh, I see your point, however I strongly doubt most people have people have the parameter list for setcookie() memorized off the top of their heads. I sure don't. :-)

In this case I think it's better to have one consistent way of doing things, and it seems like having $value be able to be a scalar or array is the best way to go here.

@mikegreiling
mikegreiling added a note Apr 23, 2013
  • updated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nateabele
Union of RAD member

Dude, I just want to say this is awesome. Thanks for all your effort on this. Most of the functionality looks great, but this is a huge patch, so naturally there are some things to work through. What I might do is merge dev to master now, then merge this, then we can work on it more once it's in a branch (or we can keep working on it here, up to you).

One thing I noticed in terms of the functionality is this:

'Header: value1',
'Header: value2',

I checked the spec, and it looks like this is invalid. When you have multiple values for the same header, they're supposed to be comma-separated. Are you working with an API that handles this rong?

@mikegreiling

Thanks. I saw you were heading to 1.0 this week, so it sorta lit a fire under my feet to get some stuff out before a milestone release.

Let's leave development in this PR for the moment... I'm working on a few squashes based on your and @jails feedback.

As for the multiple headers, I can say for sure this is allowed (and in fact required) for Set-Cookie (see RFC6265)

[...] An origin server can include multiple
Set-Cookie header fields in a single response. The presence of a
Cookie or a Set-Cookie header field does not preclude HTTP caches
from storing and reusing a response.

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into
a single header field. The usual mechanism for folding HTTP headers
fields (i.e., as defined in [RFC2616]) might change the semantics of
the Set-Cookie header field because the %x2C (",") character is used
by Set-Cookie in a way that conflicts with such folding.

I know it is also used within Lithium for Cache-Control (see \action\Request::cache()), though perhaps that was an unintended behavior? Whether that's valid or not, I have not done any research.

@mikegreiling

It looks like the way we are setting Cache-Control over multiple headers is also valid. See this stack-overflow answer and its comments: http://stackoverflow.com/a/4371395/1447303

@mikegreiling

I stopped short of implementing cookies within \action\Request and \action\Response as per #618, but I'd like to do so in a future PR pending this one's acceptance if you'll approve of it.

@nateabele
Union of RAD member

Yeah, I'm not fundamentally opposed to it, we just need to figure out how to make it BC (we already have a pattern for this, if you check out the FirePhp logger adapter).

@mikegreiling

I've made a few changes and squashed the commits. If you want to move this to dev or its own branch, that'd be great.

@nateabele nateabele merged commit 56b1ae4 into UnionOfRAD:dev Apr 23, 2013

1 check passed

Details default The Travis build passed
@mikegreiling mikegreiling deleted the pixelcog:request-response-cookies branch Apr 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.