Remove time()

Should we wrap it to drupal_time() ?

I think wrapping it in drupal_time() is a good idea. Then if we want to add some arguments later on (timezone?), we easily could.

Yay my first major core patch! Added drupal_time() to bootstrap.inc and changed all instances of time() to drupal_time().

Removed some whitespace changes and refactored drupal_time:

function drupal_time() {
  return isset($_SERVER['REQUEST_TIME']) ? $_SERVER['REQUEST_TIME'] : $_SERVER['REQUEST_TIME'] = time();
}

Does wrapping make any sense? I thought we are hunting performance here which a function call pretty much kills. Of course, you still save a call to the system but not wrapping would be a much bigger save...

If we don't want to wrap this in a function call, then the function referer_uri should be replaced with just $_SERVER['HTTP_REFERER']. It's nearly the exact same thing.

This patch replaces referer_uri() with $_SERVER['HTTP_REFERER'] and time() with $_SERVER['REQUEST_TIME'].

Committed this patch to CVS HEAD. We want to mention this in the module update documentation. Mark this 'fixed' once the documentation is updated. Good work folks! Thanks.

Ouch. Big -1 for replacing referer_uri. There was a isset() there. At least we should sanitize $_SERVER in bootstrap.inc to guarantee that $_SERVER['REFERER_URI'] is always available when we need them.

Here is a patch for the failures, that will also guarantee compatibility with PHP < 5.1.

This new patch also fixes a breakage in the session test introduced by #8. And we are back at our initial state.

Well, we requires PHP 5.2, so I was a little too conservative.

This change broke a smorgasbord of tests. :(

# Import feeds from OPML functionality: 47 passes, 2 fails, 0 exceptions
# Blog API functionality: 35 passes, 10 fails, 4 exceptions
# Comment functionality: 461 passes, 15 fails, 1 exception
# Session tests: 81 passes, 1 fail, 0 exceptions
# Top visitor blocking: 29 passes, 3 fails, 0 exceptions
# XML-RPC: passes, 9 fails, 0 exceptions

Confirmed that DamZ's patch fixed the problem. Curiously, I was having these failures under 5.2.5, so I think setting $_SERVER['request_time'] if it's not already set is a good move. But it is rather strange to do it in drupal_unset_globals(). Can we wrap this in another function or something?

After discussion with chx and webchick on IRC, we kill drupal_unset_globals() that is completely unneeded (we have a requirement that register_globals is OFF), and add drupal_initialize_variables().

Awesome. Thanks a lot, DamZ!

Back to 100% tests passing. :)

Oh, also, we need a note at http://drupal.org/node/224333 about this. We should probably also add something in a more timeless place, but I'm not sure exactly where. Coding standards?

@webchick, I created a docs issue #304990: Please add Dave Reid to documentation team so I can start helping with this.

I have fixed the CRAP out of that issue! ;) Thanks, Dave!

You sure did... Thanks Angie!

Added notes about both under http://drupal.org/node/224333. A little unsure if or where the $_SERVER['REQUEST_TIME'] should be in the coding standards...

Looks good, Dave. Thanks!

@17: when the requirement for register_globals to be OFF was introduced (http://drupal.org/drupal-5.6), it only applied to new installs, but existing sites could run with it ON albeit you get a warning on the status report. Is it not theoretically possible that a site upgraded to D7 could still be running with it ON? I notice that http://api.drupal.org/api/function/system_requirements/7 still has a test for register_globals which would be unnecessary if this situation couldn't arise.

Also (probably a separate issue) I can't see any mention of register_globals on the http://drupal.org/requirements page.

[edit] Corrected typo above - changed "It is not theoretically possible ..." to "Is it not theoretically possible ..."

@gpk: the test in system_requirements is meant to prevent install to proceed if register_globals is on. I added the requirement to http://drupal.org/requirements.

@26: yes, the requirement prevents install if register_globals is on, but what about upgrades from sites that were initially installed on 5.5 or earlier? [apologies, there was a typo in my #25 that confused the issue somewhat!]

@26, 27: So my point is that because we have now removed drupal_unset_globals(), an (unsupported) site on a server with register_globals on that had been upgraded from 5.5 or earlier to 7.x would if I'm not mistaken be extremely insecure. I think either drupal_unset_globals() needs to be restored, or else Drupal needs to be prevented from working in any circumstances on such a server (e.g. by preventing the upgrade to 7.x) in which case the register_globals check could possibly be skipped in system_requirements() when $phase == 'runtime', though it might be preferable to leave it.

@gpk: Drupal should neither install nor update if register_globals is on. The updater should stop when the _requirements are not satisfied. If it doesn't currently, we should open a new issue to make sure it does.

Thanks Damien, looks like http://api.drupal.org/api/function/update_check_requirements/6 was added in D6 hence as you say the situation should not occur :) .

funny - I filed essentially the same issue here: http://drupal.org/node/305645

however, could we have a more compact define for this? I think that would encourage contrib to use it more too. e.g.:

define ('R_TIME', $_SERVER['REQUEST_TIME']);

Please follow-up in the issue above on this idea.