Add date header to server responses #139

legionth · 2017-03-06T01:20:07Z

According to https://tools.ietf.org/html/rfc7231#section-7.1.1.2 a Date header should be added to the response message.

The date of a server will only be added when none date is given in the response header.

jsor · 2017-03-06T07:06:06Z

README.md

@@ -207,6 +207,9 @@ See [`writeHead()`](#writehead) for more details.

 See the above usage example and the class outline for details.

+A `Date` header will be automatically add the system date and time if none is given.


Sentence does not parse :) Probably something like:

A Date header will be automatically added with the system date and time if none is given.

You are right. Thank you!

jsor · 2017-03-06T07:12:25Z

README.md

@@ -207,6 +207,9 @@ See [`writeHead()`](#writehead) for more details.

 See the above usage example and the class outline for details.

+A `Date` header will be automatically add the system date and time if none is given.
+Add a custom `Date` header via `writeHead()`](#writehead).
+


The leading [ for the link is missing here:

[writeHead()](#writehead)

Change it in the newest commit.

jsor · 2017-03-06T07:17:35Z

src/Response.php

@@ -204,6 +204,15 @@ public function writeHead($status = 200, array $headers = array())
            );
        }

+        // assign date header if no 'date' is given, use the current time where this code is running
+        if (!isset($lower['Date'])) {


Must test against $lower['date'] (lowercase). Also, looks like this case isn't properly tested :)

Oh, you are right. Changed it in the newest commit.

kelunik · 2017-03-06T08:17:15Z

src/Response.php

+        if (!isset($lower['Date'])) {
+            // example: "Date: Tue, 15 Nov 1994 08:12:31 GMT"
+            $headers = array_merge(
+                array('Date' => date('D, d M Y H:i:s T')),


This should be gmdate('D, d M Y H:i:s') . ' GMT'.

Can you link to the RFC?

https://tools.ietf.org/html/rfc7231#section-7.1.1.2 links to https://tools.ietf.org/html/rfc5322#section-3.6.1 which links to https://tools.ietf.org/html/rfc5322#section-3.3

It's defined in the section just above the one you linked: https://tools.ietf.org/html/rfc7231#section-7.1.1.1

Thanks for the link!

When a sender generates a
header field that contains one or more timestamps defined as
HTTP-date, the sender MUST generate those timestamps in the
IMF-fixdate format.
[…]
Preferred format:
IMF-fixdate = day-name "," SP date1 SP time-of-day SP GMT

Thank you @kelunik and @clue . I changed the behavior in the latest commit. Check it out.

clue

An origin server MUST NOT send a Date header field if it does not
have a clock capable of providing a reasonable approximation of the
current instance in Coordinated Universal Time.

https://tools.ietf.org/html/rfc7231#section-7.1.1.2

I think this change makes perfect sense for 99% of our consumers 👍

However, the RFC specifically says one MUST NOT add a Date header if the system does not have an appropriate clock. How can I disable sending this header if my system has no such clock?

clue · 2017-03-06T08:20:56Z

tests/ResponseTest.php

+        $this->assertContains("Date: ", $buffer);
+        $this->assertContains("Transfer-Encoding: chunked\r\n", $buffer);
+        $this->assertContains("Connection: close\r\n\r\n", $buffer);
+        $this->assertContains("\r\n\r\n", $buffer);


Are these changes needed? (See also below) They add quite a bit of noise to this PR.

My vote would be that the previous tests explicitly tested receiving a complete, valid HTTP response, while the new tests only test certain header fields. What is the motivation for this?

With the newest commit, only one line changed. I removed the date from the previous tests via writeHead('Date' => array()). I think this is better readable now.

clue · 2017-03-06T08:23:51Z

src/Response.php

+        if (!isset($lower['Date'])) {
+            // example: "Date: Tue, 15 Nov 1994 08:12:31 GMT"
+            $headers = array_merge(
+                array('Date' => date('D, d M Y H:i:s T')),


Can you link to the RFC?

https://tools.ietf.org/html/rfc7231#section-7.1.1.2 links to https://tools.ietf.org/html/rfc5322#section-3.6.1 which links to https://tools.ietf.org/html/rfc5322#section-3.3

kelunik · 2017-03-06T08:31:55Z

@clue If your system has no such clock, microtime won't exist in PHP and the event loop won't run, so nothing to worry about.

clue · 2017-03-06T09:24:12Z

@kelunik microtime() will exist, however it actually returns a timestamp at an arbitrary point in time, most likely several years back and will continue to progress "normally", but will never approach the "real time". Unfortunately, this is quite common for many smaller systems, such as embedded systems or microcontroller/SOC boards.

I'm not saying this should be our main target here, but we SHOULD at least support sending NO Date header at all in these cases 👍

clue · 2017-03-06T10:51:03Z

README.md

@@ -207,6 +207,10 @@ See [`writeHead()`](#writehead) for more details.

 See the above usage example and the class outline for details.

+A Date header will be automatically added with the system date and time if none is given.
+Add a custom `Date` header via [`writeHead()`](#writehead). If you don't have a appropriate clock to
+rely on, you should unset this header with an empty array: `writeHead('Date' => array())`


Text LGTM, but should probably moved down to writeHead() 👍

Put it into the writeHead chapter, also added some examples to it

clue · 2017-03-06T11:18:04Z

src/Response.php

+            $headers = array_merge(
+                array('Date' => gmdate('D, d M Y H:i:s') . ' GMT'),
+                $headers
+            );


While this LGTM, is there a particular reason you're not using an direct array assignment here? (should be faster too)

There was no particular reason. Must be my daily condition. Changed it in the newest commit.

kelunik · 2017-03-06T11:20:50Z

@clue If HAVE_GETTIMEOFDAY is not defined, microtime isn't registered as function, see https://github.com/php/php-src/blob/c6982995504b6e21e8a5ade29cfb16a55196dc43/ext/standard/microtime.h#L24.

clue · 2017-03-06T11:25:55Z

@clue If HAVE_GETTIMEOFDAY is not defined, microtime isn't registered as function

Thanks for digging this out, but I suppose this discussion should not be really part of this PR? :)

My concern was mostly for systems that do in fact have a clock, but where the consumer of this library knows that it is always going to be out of sync, such as being several years in the past, but otherwise "normally ticking".

I think the current version should already cover this, so the changes LGTM 👍

clue

Nice, LGTM 👍

WyriHaximus · 2017-03-07T09:50:41Z

@legionth LGTM, could you resolve conflicts?

legionth · 2017-03-07T10:28:38Z

Ping @WyriHaximus . Resolved the conflicts

WyriHaximus · 2017-03-07T10:46:23Z

@legionth 👍

jsor requested changes Mar 6, 2017

View reviewed changes

kelunik suggested changes Mar 6, 2017

View reviewed changes

clue requested changes Mar 6, 2017

View reviewed changes

legionth force-pushed the date-header branch 2 times, most recently from 9b3eed7 to 49a2fe4 Compare March 6, 2017 10:31

clue reviewed Mar 6, 2017

View reviewed changes

legionth force-pushed the date-header branch from 49a2fe4 to 0911789 Compare March 6, 2017 11:01

clue reviewed Mar 6, 2017

View reviewed changes

legionth force-pushed the date-header branch from 0911789 to a137fc1 Compare March 6, 2017 11:21

clue approved these changes Mar 6, 2017

View reviewed changes

jsor approved these changes Mar 6, 2017

View reviewed changes

clue added HTTP/1.1 new feature labels Mar 6, 2017

clue added this to the v0.6.0 milestone Mar 6, 2017

WyriHaximus approved these changes Mar 7, 2017

View reviewed changes

Add system date 'Date' header if none isset

b50d7c4

legionth force-pushed the date-header branch from a137fc1 to b50d7c4 Compare March 7, 2017 10:27

WyriHaximus merged commit 5c5cb0b into reactphp:master Mar 7, 2017

clue mentioned this pull request Mar 7, 2017

Support detecting request body #104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add date header to server responses #139

Add date header to server responses #139

legionth commented Mar 6, 2017

jsor Mar 6, 2017

legionth Mar 6, 2017

jsor Mar 6, 2017

legionth Mar 6, 2017

jsor Mar 6, 2017

legionth Mar 6, 2017

kelunik Mar 6, 2017

clue Mar 6, 2017

kelunik Mar 6, 2017

clue Mar 6, 2017

legionth Mar 6, 2017

clue left a comment

clue Mar 6, 2017

legionth Mar 6, 2017

clue Mar 6, 2017

kelunik commented Mar 6, 2017

clue commented Mar 6, 2017

clue Mar 6, 2017

legionth Mar 6, 2017

clue Mar 6, 2017

legionth Mar 6, 2017

kelunik commented Mar 6, 2017

clue commented Mar 6, 2017

clue left a comment

WyriHaximus commented Mar 7, 2017

legionth commented Mar 7, 2017

WyriHaximus commented Mar 7, 2017

		@@ -207,6 +207,9 @@ See [`writeHead()`](#writehead) for more details.

		See the above usage example and the class outline for details.

		A `Date` header will be automatically add the system date and time if none is given.

Add date header to server responses #139

Add date header to server responses #139

Conversation

legionth commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelunik commented Mar 6, 2017

clue commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelunik commented Mar 6, 2017

clue commented Mar 6, 2017

clue left a comment

Choose a reason for hiding this comment

WyriHaximus commented Mar 7, 2017

legionth commented Mar 7, 2017

WyriHaximus commented Mar 7, 2017