Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Per Rasmus' talk at Drupalcon, time() can now be replaced with $_SERVER['REQUEST_TIME'] if you just want current time, which is free. I'm also getting a test failure in user.test for user created time, which I assume is probably a timezone issue on my laptop at the moment. Either way, PHP5 goodness!
Comment | File | Size | Author |
---|---|---|---|
#17 | 302763-what-is-testing-for-4.patch | 2.22 KB | Damien Tournoud |
#15 | 302763-what-is-testing-for-3.patch | 1.49 KB | Damien Tournoud |
#14 | 302763-what-is-testing-for-2.patch | 1.58 KB | Damien Tournoud |
#13 | 302763-what-is-testing-for.patch | 648 bytes | Damien Tournoud |
#8 | 302763.patch | 58.39 KB | RobLoach |
Comments
Comment #1
kika CreditAttribution: kika commentedShould we wrap it to drupal_time() ?
Comment #2
RobLoachI think wrapping it in drupal_time() is a good idea. Then if we want to add some arguments later on (timezone?), we easily could.
Comment #3
Dave ReidYay my first major core patch! Added drupal_time() to bootstrap.inc and changed all instances of time() to drupal_time().
Comment #4
RobLoachRemoved some whitespace changes and refactored
drupal_time
:Comment #5
chx CreditAttribution: chx commentedDoes 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...
Comment #6
RobLoachComment #7
Dave ReidIf 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.
Comment #8
RobLoachThis patch replaces referer_uri() with $_SERVER['HTTP_REFERER'] and time() with $_SERVER['REQUEST_TIME'].
Comment #9
Dries CreditAttribution: Dries commentedCommitted 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.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedOuch. 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.
Comment #11
catchedit: no it didn't.
Comment #12
catchI had some test failures, but it seems to be a local issue, false alarm, nothing to see here.
If you visit a page directly, you get undefined index in bootstrap.inc due to the missing isset() for referrer.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is a patch for the failures, that will also guarantee compatibility with PHP < 5.1.
Comment #14
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis new patch also fixes a breakage in the session test introduced by #8. And we are back at our initial state.
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedWell, we requires PHP 5.2, so I was a little too conservative.
Comment #16
webchickThis 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?
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedAfter 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().
Comment #18
webchickAwesome. Thanks a lot, DamZ!
Back to 100% tests passing. :)
Comment #19
webchickOh, 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?
Comment #20
Dave Reid@webchick, I created a docs issue #304990: Please add Dave Reid to documentation team so I can start helping with this.
Comment #21
webchickI have fixed the CRAP out of that issue! ;) Thanks, Dave!
Comment #22
Dave ReidYou sure did... Thanks Angie!
Comment #23
Dave ReidAdded 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...
Comment #24
webchickLooks good, Dave. Thanks!
Comment #25
gpk CreditAttribution: gpk commented@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 ..."
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #27
catchSites running with register_globals on would be unsupported. Whether we need to explicitly check for it in D7 I'm not sure.
Comment #28
gpk CreditAttribution: gpk commented@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.
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #30
gpk CreditAttribution: gpk commentedThanks 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 :) .
Comment #31
pwolanin CreditAttribution: pwolanin commentedfunny - 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.:
Please follow-up in the issue above on this idea.
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.