http://php.net/manual/en/function.time.php

We were using REQUEST_TIME as a constant, which it is not (at least not when we execute code). I've replaced all naked REQUEST_TIME and time() instances with $_SERVER['REQUEST_TIME'].

The nice side effect, apart from removing a warning, is that it fixes getMulti implementation and boosts the cache hit rate from 5% to approaching 100%.

CommentFileSizeAuthor
#13 925096-remove-debugging-memcache-session.inc_.patch.txt711 bytessillygwailo
#10 reverse_request_time.patch3.83 KBrobertdouglass
#4 request_time.patch5.37 KBrobertdouglass
#3 request_time.patch3.75 KBrobertdouglass
#1 request_time.patch10.06 KBrobertdouglass

Comments

robertdouglass’s picture

robertdouglass commented
StatusFileSize
new request_time.patch10.06 KB

For those who want a patch.

robertdouglass’s picture

robertdouglass commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)

#925096 by robertDouglass: Fixed Use of REQUEST_TIME improper, incomplete.
Committing to 6.x-1.x.

robertdouglass’s picture

robertdouglass commented
Status: Patch (to be ported) » Fixed
StatusFileSize
new request_time.patch3.75 KB

#925096 by robertDouglass: Fixed Use of REQUEST_TIME improper, incomplete.
committed to D7 as well.

robertdouglass’s picture

robertdouglass commented
StatusFileSize
new request_time.patch5.37 KB

Patched and committed to DRUPAL-5 for completeness.

swentel’s picture

swentel commented

@robertdouglass : shouldn't memcache for D6 than be labeled in the info file for a least PHP > 5.1.x (although that's stupid since it doesn't load the module file at all) because $_SERVER['REQUEST_TIME'] is not available before that PHP version. You never know that someone comes complaining :) Or maybe a warning on the project page is enough ?

robertdouglass’s picture

robertdouglass commented

Yes, good point. I'll add that to the documentation and .info files.

robertdouglass’s picture

robertdouglass commented
Status: Fixed » Needs work

These patches aren't totally correct either. bootstrap.inc does this:

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

I don't fully understand why I was getting cases of REQUEST_TIME not being set. Will investigate further, and possibly remove some of the $_SERVER instances as it seems unnecessary.

catch’s picture

catch
he/him
Primary language English
commented

Core only does this for D7, in D6 there's no constant - which version were you seeing this on?

robertdouglass’s picture

robertdouglass commented

I was seeing it on D6. Now that I'm clear about the difference I'll audit both versions to make sure it's all proper.

robertdouglass’s picture

robertdouglass commented
StatusFileSize
new reverse_request_time.patch3.83 KB

I've reversed the changes made to D7 - all instances are now just REQUEST_TIME using the bootstrap.inc constant.

robertdouglass’s picture

robertdouglass commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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

sillygwailo’s picture

sillygwailo
Location Toronto, ON
commented
Priority: Critical » Minor
Status: Closed (fixed) » Active
StatusFileSize
new 925096-remove-debugging-memcache-session.inc_.patch.txt711 bytes

The latest patch committed to the D7 branch appears to have a debugging statement in _drupal_session_write().

sillygwailo’s picture

sillygwailo
Location Toronto, ON
commented
Status: Active » Needs review
robertdouglass’s picture

robertdouglass commented
Status: Needs review » Fixed

Thanks Richard. Committed to 7.x-1.x

Status: Fixed » Closed (fixed)

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