Replaced time() with $_SERVER['REQUEST_TIME']

[ghost]

As discussed in 7bb95df#commitcomment-881705

Replacement of time() calls with $_SERVER['REQUEST_TIME'] , I left out the date_helper for now.

[it-can]

http://php.net/manual/en/reserved.variables.server.php

"REQUEST_TIME": The timestamp of the start of the request. Available since PHP 5.1.0. It is float with microseconds since PHP 5.4.0.

Does this pull request give a problem with PHP 5.4?

[ghost]

Regarding to the changelog of 5.4:

Changed $_SERVER['REQUEST_TIME'] to include microsecond precision. (Ilia)

But I have no possibility to test this with PHP 5.4 :/

I could add a floor to it, but I guess it will vanish the performance improvement. :/

[it-can]

In versions before 5.4 the request_time is a int, but from PHP 5.4 it will be a float (eg. 1326987371.123123)

then time() is maybe better to use... Maybe define a time() somewhere in the code (index.php), so you don't have to call time() multiple times?

[ghost]

Yes, I got this the first time.

Basically PHP 5.4 $_SERVER['REQUEST_TIME'] has microtime(TRUE); as value.

I tested

print(date("c", microtime(TRUE)));

which works perfectly fine.

I ran also some other test, which had no problems with the float variant.

php -r '$now = microtime(TRUE); print($now . PHP_EOL);print(mktime(gmdate("H", $now), gmdate("i", $now), gmdate("s", $now), gmdate("m", $now), gmdate("d", $now), gmdate("Y", $now)));'

php -r 'print(microtime(TRUE)-14440);'

[narfbg]

This can only break something.

[ghost]

I will test this further on the weekend, probably with a PHP 5.4 RC version and make the appropiate additions if necessary or drop the changes completely.

[it-can]

Haha, the new PHP 5.4 RC6 release (yesterday), https://svn.php.net/repository/php/php-src/tags/php_5_4_0RC6/NEWS

_Restoring $_SERVER['REQUEST_TIME'] as a long and introducing $SERVER['REQUEST_TIME_FLOAT'] to include microsecond precision.

So your pull request should work ;-)

[ghost]

oh oops, I could have re-opened ... :/ sorry, I am new to this. =)

[flashover]

What about using $this->input->server('REQUEST_TIME') for each $_SERVER['REQUEST_TIME'] ?

[ghost]

Well, this would make sense if the client actually had access to this global. But it's filled by the php process with time() on the beginning of the request processing.

[narfbg]

@flashover The point is to not have the overhead of calling a function and use a variable instead - calling a custom method (that reads from this very same variable) doesn't make any sense.

I still think this is not needed though. I'd usually do crazy stuff to optimize for speed, but time() is one of the fastest functions available and $_SERVER is not a constant - it's elements' values can be manipulated both prior and after it's initialization, so I'm not sure if relying on it is 100% safe.

[ghost]

@narfbg

If something or someone is modifying $_SERVER in your app, you will have other problems than performance. :)

So, this pull request is rejected then?

[narfbg]

@TwoBee

Indeed, but using time() makes that impossible, at least in those particular cases. :)
But ... I don't make the decisions of what's merged and rejected and unfortunately sometimes that seems to take too long to be done.

[philsturgeon]

I am listening guys, I was just waiting for you two to hash out your argument as I am on the fence. So it seems, this change will work in 5.1-5.3, but COULD cause problems in 5.4 due to the int/float change.

Test it, does it cause problems. If so, typecasting for the win?

@narbg I don't think we need to worry about security here, nobody is going to be messing around with the $_SERVER variable and if they are they deserve any confusion they get. I also don't think this change is about speed differences of variable access over function calls, it is about reporting the time more accurately on slow page calls.

[narfbg]

@philsturgeon

Um, no - the int/float issue only exist in beta and some of the RC versions for PHP 5.4. This is a quote from the changelog for 5.4.0 RC6:

Restoring $_SERVER['REQUEST_TIME'] as a long and introducing $_SERVER['REQUEST_TIME_FLOAT'] to include microsecond precision. (Patrick)

IMO, time accuracy is a completely different argument, as in some cases it might be more convenient to have records for when did a certain process take place instead of when the request was initiated. Actually, now that you've mentioned that ... most of the changed libraries are only useful in web applications, but e.g. the DB utility class could be used via CLI.

Let's say we have a daemon like script that does database backups every n hours (it's stupid, because that should be done via crontab, but still possible and in this case - just an example). And here's what happens:

• a PHP process is started/forked and $_SERVER['REQUEST_TIME'] is initiated with the value of time()
• after a few hours, it's time to make a backup of the database
• the developer has not specified a custom name for the output file and the DB utility class creates one based on a timestamp
• each time the backup procedure is triggered, due to $_SERVER['REQUEST_TIME'] not being changed - the same timestamp-based filename is overwritten

A similar issue can be expected with the ZIP and probably Image manipulation libraries.

[philsturgeon]

Seems valid. I'll be honest, I don't really care either way about this
one, I am just looking for the best side to pick based on the most well
reasoned argument.

So far Andrey seems to have the best reasoning, so "Against" is winning.

---

Andrey Andreev
mailto:reply@reply.github.com
25 January 2012 03:03

@philsturgeon

IMO, time accuracy is a completely different argument, as in some
cases it might be more convenient to have records for when did a
certain process take place instead of when the request was initiated.
Actually, now that you've mentioned that ... most of the changed
libraries are only useful in web applications, but e.g. the DB utility
class could be used via CLI.

Let's say we have a daemon like script that does database backups
every n hours (it's stupid, because that should be done via crontab,
but still possible and in this case - just an example). And here's
what happens:

• a PHP process is started/forked and $_SERVER['REQUEST_TIME'] is
  initiated with the value of time()
• after a few hours, it's time to make a backup of the database
• the developer has not specified a custom name for the output file
  and the DB utility class creates one based on a timestamp
• each time the backup procedure is triggered, due to
  $_SERVER['REQUEST_TIME'] not being changed - the same
  timestamp-based filename is overwritten

A similar issue can be expected with the ZIP and probably Image
manipulation libraries.

---

Reply to this email directly or view it on GitHub:

#944 (comment)

Tobias Mathes
mailto:reply@reply.github.com
19 January 2012 07:59

As discussed in
7bb95df#commitcomment-881705

Replacement of time() calls with $_SERVER['REQUEST_TIME'] , I left out
the date_helper for now.

You can merge this Pull Request by running:

git pull https://github.com/twobee/CodeIgniter replace_time

Or you can view, comment on it, or merge it online at:

#944

-- Commit Summary --

• + system/core/Input.php: replaced time()
• + system/core/Output.php: replaced time()
• + system/core/Security.php: Replaced time()
• + system/database/DB_utility.php: Replaced time()
• + system/libraries/Calendar.php: Replaced time()
• + system/libraries/Image_lib.php: Replaced time()
• + system/libraries/Session.php: Replaced time()
• + system/libraries/Zip.php: Replaced time()
• + system/libraries/Cache/drivers/Cache_apc.php: Replaced time()
• + system/libraries/Cache/drivers/Cache_file.php: Replaced time()
• + system/libraries/Cache/drivers/Cache_memcached.php: Replaced time()
• + user_guide_src/source/general/models.rst: Replaced time() in examples
• + user_guide_src/source/helpers/captcha_helper.rst: Replaced time()
  in examples

-- File Changes --

M system/core/Input.php (4)
M system/core/Output.php (2)
M system/core/Security.php (2)
M system/database/DB_utility.php (2)
M system/libraries/Cache/drivers/Cache_apc.php (2)
M system/libraries/Cache/drivers/Cache_file.php (2)
M system/libraries/Cache/drivers/Cache_memcached.php (4)
M system/libraries/Calendar.php (2)
M system/libraries/Image_lib.php (2)
M system/libraries/Session.php (7)
M system/libraries/Zip.php (8)
M user_guide_src/source/general/models.rst (6)
M user_guide_src/source/helpers/captcha_helper.rst (4)

-- Patch Links --

https://github.com/EllisLab/CodeIgniter/pull/944.patch
https://github.com/EllisLab/CodeIgniter/pull/944.diff

---

Reply to this email directly or view it on GitHub:
#944

[ghost]

Because of the accuracy thing I left out the Date Helper in the pull request. But usually on high traffic sites you will need 'minor' win this change is providing.

@narfbg
I really love your example, it's a good example how not to do the job for such kind of backup mechanism. :)

We could leave out the DB_utility and Calendar, if this is the issue. :)

[narfbg]

If it's not used in DB utility, Image_lib and Zip (or in other words - if it's only applied to HTTP-only libraries) - I'm OK with it. Indeed it's more a matter of preference there and I'm probably way too concerned about security at times. :)

[narfbg]

@TwoBee Seems like we replied at the same time. :)

Calendar generates HTML code and is useless in the CLI, so it's good in there.

[ghost]

@narfbg

As I mentioned before, if someone messes with your $_SERVER you really have other (more serious) problems.
Usually I agree with pro security measures, but in this case I thinks it's missing the point. :)

Yes, I agree on a CLI level this could lead to a certain inaccurancy.

I can revoke it or we discuss which measure are useful and doing just them instead of all. I would then apply this changes only to my system. :)

[ghost]

I will update the repository later this week to the changes we discussed.

[it-can]

What's the status @TwoBee ?

[ghost]

I will update the request within the next two days. :)

[ryanneufeld]

Radical thought here:

Why not just have in the constants config file

<?php
define('REQUEST_TIME', time());

then use that?

[narfbg]

Probably because a variable with the same value already exists?

[ryanneufeld]

The variable exists in the $_SERVER super global. Which as Phi pointed
out is mutable.

The suggestion I'm making is to create a define at the start of the
script with the script request time. The name isn't as relevant as
it's purpose, which is to provide a consistent time value for the
duration of script execution.

On the topic of needing exact second correctness in the DB etc. the
programmer implementing it can just store the value of time() instead
of using the define.

--Ryan Neufeld BSc CIS
ryan@neufeldmail.com

On Fri, Mar 30, 2012 at 7:34 AM, Andrey Andreev
reply@reply.github.com
wrote:

Probably because a variable with the same value already exists?

---

Reply to this email directly or view it on GitHub:
#944 (comment)

[ghost]

@ryanneufeld Still, it's missing the whole point here.

[ghost]

@it-can Sorry, got a bit tied up in the last time :(

[narfbg]

@ryanneufeld Actually, it was me pointing out all of the cons against using $_SERVER['REQUEST_TIME'].
But apart from my own arguments, I'd rather see this one as a performance improvement than anything to do with precision.

[ryanneufeld]

@TwoBee Well, perhaps I'm missing the point. But I've seen two arguments here. What is the point then?

[ghost]

@ryanneufeld We discussed this topic more than enough, I will revert the changes as discussed with @narfbg . Then it will be either merged or not.

[ghost]

I finally applied the changes, do I need anything else to do? :)

[narfbg]

So far, so good. But why are file permissions for the cache drivers changed?
I'm also not sure about replacing time() in the documentation.

[ghost]

I can revert the examples, too. Sorry about the permissions, I guess my VMWARE settings are responsible for this. I try to fix it. :(