API page: http://api.drupal.org/api/drupal/includes--bootstrap.inc/constant/REQUES...

Describe the problem you have found:

REQUEST_TIME is a float with microseconds...

No. $_SERVER['REQUEST_TIME'] may be a float, but the REQUEST_TIME described on this page is an int. Simply in seconds, not microseconds.

Files: 
CommentFileSizeAuthor
#19 request-time-1289208-19.patch898 bytesrocket_nova
PASSED: [[SimpleTest]]: [MySQL] 33,999 pass(es).
[ View ]
#16 1289208-RequestTime-16.patch1.08 KBskottler
PASSED: [[SimpleTest]]: [MySQL] 33,270 pass(es).
[ View ]
#14 D8-1289208-RequestTime-15.patch902 bytesminorOffense
PASSED: [[SimpleTest]]: [MySQL] 33,145 pass(es).
[ View ]
#8 1289208-RequestTime-5039386.patch917 bytesminorOffense
PASSED: [[SimpleTest]]: [MySQL] 33,139 pass(es).
[ View ]
#5 1289208-RequestTime (Corrected).patch860 bytesminorOffense
PASSED: [[SimpleTest]]: [MySQL] 33,152 pass(es).
[ View ]
#2 1289208-RequestTime.patch725 bytesminorOffense
PASSED: [[SimpleTest]]: [MySQL] 36,034 pass(es).
[ View ]

Comments

Dave Cohen’s picture

Title:Documentation problem with Reply to comment» REQUEST_TIME documentation is misleading
minorOffense’s picture

Status:Active» Needs review
StatusFileSize
new725 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,034 pass(es).
[ View ]

Here's a patch with some new text.

bleen18’s picture

Status:Needs review» Needs work
+++ b/includes/bootstrap.incundefined
@@ -193,8 +193,9 @@ define('LANGUAGE_RTL', 1);
+ * $_SERVER['REQUEST_TIME'] is a float with microseconds since PHP 5.4.0, but float
+ * timestamps confuses most of the PHP functions (including date_create()). We
+ * store an integer version of this number in a constant REQUEST_TIME.

I would reverse this:

"REQUEST_TIME is an integer measuring seconds. This differs from $_SERVER['REQUEST_TIME'], which is stored as a float since PHP 5.4.0, because float timestamps confuse most PHP functions (including date_create()).

If you prefer to keep the doc as-is, then please fix a couple minor grammatical errors:

$_SERVER['REQUEST_TIME'] is a float with microseconds since PHP 5.4.0, but float timestamps confuse most PHP functions (including date_create()). We store an integer version of this number in the REQUEST_TIME constant.

3 days to next Drupal core point release.

jhodgdon’s picture

If you are going to fix this documentation, could you also bring the entire docblock up to the standards in
http://drupal.org/node/1354 - thanks! In particular, the first line is not right.

Also I agree with the review above. :)

minorOffense’s picture

StatusFileSize
new860 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,152 pass(es).
[ View ]

Yeah the new sentence is better.

I didn't see the newest comment regarding the document standard. I'll do one more (Ignore the patch attached to this comment) ;-)

minorOffense’s picture

So something more like this?

<?php
/*
  * Defines a short form of the request time global as an integer measured in seconds.
  *
  * This differs from $_SERVER['REQUEST_TIME'], which is stored as a float
  * since PHP 5.4.0, because float timestamps confuse most PHP functions
  * (including date_create()).
  */
?>
jhodgdon’s picture

That looks better, yes! The only thing that needs to be changed is that each line should wrap as close to 80 characters as possible without going over, and if the first line is longer than 80 characters, it should be shortened. It's allowable for constants' one-line summaries to start with something other than a verb (for instance, it could say just "Short form of the ..." rather than "Defines the short form...").

minorOffense’s picture

StatusFileSize
new917 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,139 pass(es).
[ View ]

Shortened and updated.

Dave Cohen’s picture

To my reading, "short form of the request time global as an integer measured in seconds," is not really a proper sentence. How about...

This constant is the time of the current request measured in the number of seconds since the Unix Epoch (January 1 1970 00:00:00 GMT).

And perhaps add a sentence....

It is often preferable to use REQUEST_TIME rather than time(), because it is more performant, and data with the same REQUEST_TIME can be correlated to the same request.

jhodgdon’s picture

I would strongly avoid "This constant is a ..." in the first line of this docblock. We don't do that with functions, and we shouldn't do that with constants either. But you're right that it's not a sentence. I guess we're back to starting with a verb like we do with functions.

Second sentence - if it's necessary, put it in.

Dave Cohen’s picture

Agreed we should keep the doc style consistant. I feel "measured in seconds" is not really informative, but "the number of seconds since the Unix Epoch..." means something. And people should know this constant is an alternative to calling time(). I leave it up to you guys how to put that in words.

Although to tell you the truth, all my experience is on apache on linux where I'm pretty sure $_SERVER['REQUEST_TIME'] is seconds sinch the Unix Epoch. I have no idea whether other servers use the same value. I'm looking at you, IIS.

minorOffense’s picture

I'll give it another go.

Should we put the entire explanation of the epoch and $_SERVER['REQUEST_TIME'] in though? Could we not just put a @see to the docs on php.net explaining it all?

I would argue that the second sentence should be there since when I read the doc initially, my first thought was "well why not just use the server time?"
The second sentence gives me a reason to use this instead since I wasn't aware of the change to the variable PHP 5.4

jhodgdon’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Definitely, link to php.net rather than trying to duplicate its explanation of server time.

And I'm in favor of the second sentence if it answered a burning question you had after reading the initial doc. That's why we write doc, to answer those burning questions. :) How about a new patch?

And by the way, this needs to be (technically) patched in 8.x first and then backported to 7.x (although at this point in the D8 dev cycle, mostly "backport" means that the patch can just be applied to D7).

minorOffense’s picture

Status:Needs work» Needs review
StatusFileSize
new902 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,145 pass(es).
[ View ]

Alright, so here's the updated block (third time's the charm ;-)

1) Text

<?php
/**
 * Time of the current request measured in the number of seconds since the
 * Unix Epoch
 *
 * This differs from $_SERVER['REQUEST_TIME'], which is stored as a float
 * since PHP 5.4.0. Float timestamps confuse most PHP functions
 * (including date_create()).
 *
 * @see 'http://php.net/manual/en/reserved.variables.server.php'
 * @see 'http://php.net/manual/en/function.time.php'
 */
define('REQUEST_TIME', (int) $_SERVER['REQUEST_TIME']);
?>

The quotes around the URLs in the doc block aren't in the patch. They're just there to stop the text formatter from turning them into links.

The bit about using it as an alternative to time() I think would be better suited as a comment on the API site instead of part of the docs themselves since it is just a suggestion. In some cases the developer may want to call time() instead and we don't want to discourage that.

I put two @see references since they both discuss the $_SERVER['REQUEST_TIME'] variable.

bleen18’s picture

Status:Needs review» Needs work
+++ b/includes/bootstrap.incundefined
@@ -191,10 +191,15 @@ define('LANGUAGE_LTR', 0);
+ * Unix Epoch

needs a period (.)

-2 days to next Drupal core point release.

skottler’s picture

StatusFileSize
new1.08 KB
PASSED: [[SimpleTest]]: [MySQL] 33,270 pass(es).
[ View ]

I've attached a patch with the trailing period following "Unix Epoch".

skottler’s picture

Status:Needs work» Needs review

Forgot to change status.

jhodgdon’s picture

Status:Needs review» Needs work

A couple of issues here:

define('WATCHDOG_DEBUG', 7);
-
/**
  * @} End of "defgroup logging_severity_levels".
  */

Why remove that line? I think it belongs there for readability.

/**
- * For convenience, define a short form of the request time global.
+ * Time of the current request measured in the number of seconds since the
+ * Unix Epo...

The first line of any docblock is one sentence less than 80 characters on one line.

+ * @see http://php.net/manual/en/reserved.variables.server.php
+ * @see http://php.net/manual/en/function.time.php

We like to avoid linking directly to the English-specific version of the PHP manual. There are other links to php.net that avoid this problem in the Core code. Please follow their example.

rocket_nova’s picture

Assigned:Unassigned» rocket_nova
Status:Needs work» Needs review
StatusFileSize
new898 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,999 pass(es).
[ View ]

Patch from #16 re-rolled based on feedback in #18.

bleen18’s picture

Status:Needs review» Reviewed & tested by the community

SOLD!!

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 7.x and 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.