Problem/Motivation
The $user->cache
property is used to support a per-user minimum cache lifetime, but the property does not exist in a newly-created or newly-logged-in $user object. This causes various unit tests to fail, because the caching system assumes the $user->cache
always exists. Quoting jose.guevara in #48:
entity_load() is based on the schema, but User doesn't have a cache property in the schema. So the newly created user lacks the cache property.
Now why does it mess up just at login, and nowhere else?
Thanks to _drupal_session_write(). When the user's session is saved, this function checks for $user->cache, if not set, it assigns 0 to it and saves the session. When the session is read for the subsequent page, the user object is populated with all the columns from the session's table. Which of course has a cache column defined. Therefore the user obj now has its cache property.
Proposed resolution
After some discussion regarding the best way to ensure that $user->cache
always exists for a newly-created user, Damien Tournoud responded in #76
Let's not add another plaster on a wooden leg: the minimum cache lifetime feature is utterly broken, ... Don't use it. Ever.
If we want to fix it properly, we would need something among the lines of this patch: $user->cache should just not exist, and this information should be stored in the session itself.
Damien attached a working implementation as 1015946-repair-minimum-cache-lifetime.patch.
A test demonstrating the problem and the fix is included with the patch in #143.
Remaining tasks
The patch needs to be reviewed by a competent core maintainer and committed to 8.x.
For 7.x we should choose between the following:
-
Set
$user->cache
in bothuser_login_finalize()
andDrupalWebTestCase::setUp()
.
(See fix-per_user-cache-expiration-1015946-110-1-d7.patch.) -
Backport the 8.x patch.
(See fix-per_user-cache-expiration-1015946-114-d7.patch).It'd be a small API change for D7 cache backends, as a memcache co-maintainer I would not mind that API change.
User interface changes
None
API changes
The $user->cache
property is eliminated in favor of storing/retrieving $_SESSION['cache_expiration][$bin] directly.
The related {session}.cache
field is also eliminated.
Original report by antiorario
When I log in, I get the notice "Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 395 of /path/to/drupal7/includes/cache.inc)." It happened also when I was on RC3.
Anyone else?
Comment | File | Size | Author |
---|---|---|---|
#168 | cache-expiration-1015946-168.patch | 6.5 KB | pillarsdotnet |
#166 | cache-expiration-1015946-166.patch | 6.48 KB | pillarsdotnet |
#163 | cache-expiration-1015946-163.patch | 6.44 KB | pillarsdotnet |
#161 | cache-expiration-1015946-161.patch | 7.5 KB | pillarsdotnet |
#160 | cache-expiration-1015946-160.patch | 8.77 KB | pillarsdotnet |
Comments
Comment #1
letsrok CreditAttribution: letsrok commentedDitto. Latest release 7.0.
otice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 395 of ...../includes/cache.inc).
Comment #2
Jej CreditAttribution: Jej commentedYep, I get the same notice (since RC-2, not tested before).
I thought it was due to the migration from Wordpress to D6 (using wp import module) then to D7-RC2, so I did not report it. Did you migrate too?
Jej
Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 395 of D7PATH/includes/cache.inc).
Comment #3
antiorario CreditAttribution: antiorario commentedMine was a clean install of RC3, now updated to 7.0.
Comment #4
droplet CreditAttribution: droplet commentedif it can be reproduce with a clean & fresh installation, we can consider it as bug. (need more info how to reproduce this bug and phpinfo, system info..etc)
Comment #5
knarzer77 CreditAttribution: knarzer77 commentedI have the same problem. I have made an upgrade from Drupal 6 on a xampp test system (php 5.3.1).
Jan
Comment #6
teiko CreditAttribution: teiko commentedSame on my Side
Greetings
Comment #7
Jej CreditAttribution: Jej commentedA patch to solve this, but needs review.
Jej
Comment #9
sp_key CreditAttribution: sp_key commentedI get exactly the same error message.
@droplet Not sure how we can help more on instructions on reproducing the error. In my situation, you log in and receive the error straight away.
I've got the following modules installed:
Chaos Tools Suite
Pathauto
Service Links
WYSIWYG
Views
OS: Linux
PHP version: 5.2.10
Comment #10
bfroehle CreditAttribution: bfroehle commentedJej: I think this is the logic that should be applied in your patch. A bigger concern though is why $user->cache isn't getting loaded properly. Do you have any modules installed which you think might be causing it? Setting to needs review to trigger the testbot.
Comment #11
Jej CreditAttribution: Jej commentedbfroehle: "why $user->cache isn't getting loaded properly"
yes, that's why I was not quite sure of this solution. I did it to avoid the message on my side... I don't know what module can cause the problem.
I use:
Jej
Comment #12
vadimc CreditAttribution: vadimc commentedthanks, 1015946-10.patch sorted it for me
Comment #13
Balu ErtlNow I succeed to eliminate this message without using the patch. On admin/config/development/performance I've just set back 'Minimum cache lifetime' and 'Expiration of cached pages' settings to <none> from the option list, cleared the cache by clicking the button above them, logged out, then back, now it does not appears.
Running D7 on localhost with XAMPP in Maintenance Mode.
Comment #14
Jej CreditAttribution: Jej commentedI confirm (#13, balu.ertl). I use D7 (updated from D7-RCx, migrated from D6)
Comment #15
parasolx CreditAttribution: parasolx commented#13 not solve problem for me.
i'm upgrading from D6.20 to D7 (Stable)
Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 395 of /home/parasolx/domains/parasolx.net/public_html/includes/cache.inc).
Comment #16
parasolx CreditAttribution: parasolx commentedpatch #10 solve the problem. no more error with enable or disable "Minimum cache lifetime" + "Expiration of cached pages".
thanks.. i think it should be mark as reviewed and tested by the community.
Comment #17
wOOge CreditAttribution: wOOge commentedI started getting this same error when I disabled caches, but left the lifetime values as not "none".
#13 Solved the problem for me as well.
Comment #18
mrintegrity CreditAttribution: mrintegrity commentedComment #19
traceelements CreditAttribution: traceelements commented#13 worked for me as well.
Comment #20
f0x.wls CreditAttribution: f0x.wls commentedSame here. I'm using fresh D7 and this one hits me. #13, balu.ertl method's worked for me :)
Thank you.
Comment #21
youropinioncounts CreditAttribution: youropinioncounts commentedDitto for #13. Fresh installation, with only the captcha and rss permissions modules installed.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is nearly won't fix. Some modules in your installations are messing up with the global user object in a way that breaks the minimum lifetime feature of Drupal. You will have to hunt which module is responsible and ask the maintainer to fix it.
Hiding the error under the carpet is the worse solution.
Comment #23
jaxon77 CreditAttribution: jaxon77 commentedI get this error when attempting to login with an email address after installing LoginToboggan
Comment #24
solomojo CreditAttribution: solomojo commentedSame error here. Fresh, clean install of Drupal 7.0. No captcha enabled.
Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 395 of [..]/includes/cache.inc).
Comment #25
solomojo CreditAttribution: solomojo commentedPatch found on post #10 worked for me.
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commentedStill cannot reproduce.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedthanks, this comment helped me.
Comment #28
dakrcz CreditAttribution: dakrcz commentedpost #10 - works for me too
Comment #29
Balu ErtlSorry Damien, I just forgot to mention that I posted not a solution, but only a work-around for the issue.
Comment #30
bfroehle CreditAttribution: bfroehle commented@balu.ertl: I think Damien was referring not to your work-around but instead to the proposed patches in #7/#10 which just hide the issue of $user->cache not being defined. :)
Comment #31
triple5 CreditAttribution: triple5 commented#7: cache.inc_.patch queued for re-testing.
Thanks for the solution and patch #10. Can we have this thread closed, so that we don't get another 100 "me too messages?"
Comment #32
RumpledElf CreditAttribution: RumpledElf commentedI started getting this too once I set the minimum cache lifetime to something other than none.
Clean d7 site with barely a handful of core modules enabled, and a d7 site with heirarchical select, APC, taxonomy manager and admin menu enabled.
Comment #33
sp_key CreditAttribution: sp_key commentedAny new developments on this one?
Has anyone identified the root of the problem?
Comment #34
kriskhaira CreditAttribution: kriskhaira commentedIs this an old issue that cropped up again in Drupal 7? While doing some research on this problem, I found this post on pastebin.com dated July 14 2010.
Comment #35
kriskhaira CreditAttribution: kriskhaira commentedI've been having the same problem.
To reproduce:
1. Go to Configuration > Development > Performance
2. Set "Minimum cache lifetime" to ANY value other than None
3. Log out and log back in. The error appears after logging in.
The other settings on that page seem to be irrelevant to this problem. The moment I set "Minimum cache lifetime" back to None, the problem disappears
Comment #36
kriskhaira CreditAttribution: kriskhaira commentedOk, here's something interesting. After fixing this cache-related problem with jPlayer on my site, this problem went away and I can now set "Minimum cache lifetime" to any value other than None without errors appearing.
As a tip to others, you might want to check if a module might be causing the problem - switch off/on your modules on D7 until you identify the module.
Comment #37
aldim CreditAttribution: aldim commentedthanks, post #10 solved the problem.
Comment #38
bfroehle CreditAttribution: bfroehle commentedI'm resetting the issue status to "Active". The most recent proposed patch in #10 is nothing but a temporary workaround.
The real issue is that some other module(s) are messing up the global $user...
After looking into this some more, I'm tempted to mark the issue "won't fix" as well.
Comment #39
Screenack CreditAttribution: Screenack commentedI am intractably seeing this issue warning about "Undefined property: stdClass::$cache". I gave #38 the benefit of the doubt and disabled all but core modules, cleared all caches, but I still see the issue. Updating includes/cache.inc as per #10 fixed what ailes my system as well.
Comment #40
xcession CreditAttribution: xcession commentedIf someone were able to identify which other module was polluting $user and post the link to the module here then we could go and badger the module owner instead.
Its always tempting to "wont fix" a bug which isn't yours, but you'll only continue to get the reports unless you redirect the blame to the real culprit. As this is the first google hit for the error message it would make sense to link the problem module here, even if this has been answered elsewhere.
Comment #41
Balu Ertlto @xcession at #40: I spent an amount of time on testing my installed contrib (then core's too) modules. My steps were:
I'm not sure is this the process that you meant or not, but I hope the following list of tested modules may helps:
to @bfroehle at #10: thanks for the patch, after testing each modules, finally it solved the problem for me too! :)
to others running their Drupal on XAMPP: Applying a patch manually (http://drupal.org/node/534548) is a working way to implement as short patches as @bfroehle's 1015946-10.
Comment #42
steenbob CreditAttribution: steenbob commentedI'm also having this problem. The only contributed modules I have in common with #41 are...
So the odds are that it's one of those four.
For me, this problem did not happen right away. I only saw this behavior several hours after I upgraded our site. There's a possibility that what causes this problem is related to a cron task that one of these contributed modules launches.
Thanks!
Comment #43
neoglez CreditAttribution: neoglez commentedMm, it seems this is valid also for D7.2, see #1197718: Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem().
Comment #44
rayvtirx CreditAttribution: rayvtirx commentedive had this same issue in all my drupal installations - using 7.4 now. even when all the modules its possible to turn off are turned off
Comment #45
Balu ErtlI manually implemented bfroehle's patch #1015946-10 both on development (at local) and production (at remote) environments (all v7.4) and the 'Undefined property: stdClass::$cache [...]' error message disappeared from DB-log after logging in.
Comment #46
rogical CreditAttribution: rogical commentedI'm using 7.4, also get this error:
Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 399 of /opt/difang/prod/qlkaixin/includes/cache.inc).
Comment #47
damd CreditAttribution: damd commentedThe patch in post #10 worked for me too. Drupal 7.4.
Comment #48
jose.guevara CreditAttribution: jose.guevara commentedHi all,
This bug has been annoying me for a while, I ignored it until now but I finally took a look at it to better understand what happens.
The issue happens at login. When the user go from an anonymous user to an auth'ed user.
if you take a look at drupal_anonymous_user(). You will notice that anonymous user as ->cache set to 0.
When a user logs in, the system performs its different validations (non-blocked user, valid password, flooding, etc..), after it all goes through, user_login_submit() loads and assign the newly auth'ed user to the global $user variable:
user_login_submit()
user_load()
user_load_multiple()
entity_load()
UserController()
entity_load() is based on the schema, but User doesn't have a cache property in the schema. So the newly created user lacks the cache property.
Now why does it mess up just at login, and nowhere else?
Thanks to _drupal_session_write(). When the user's session is saved, this function checks for $user->cache, if not set, it assigns 0 to it and saves the session. When the session is read for the subsequent page, the user object is populated with all the columns from the session's table. Which of course has a cache column defined. Therefore the user obj now has its cache property.
I decided to add the one line of code under the foreach loop used to load user pictures because I didn't want to create an other unnecessary loop.
JG
ps: Sorry #10. Your patch works. But it disturbed me not knowing what happened, and that somehow the user object did not seem to be right.
Comment #49
bfroehle CreditAttribution: bfroehle commentedJG: Thanks for the analysis. I think you're probably right on track here. I'm going to set the issue status to needs review to move the process along.
Comment #51
Adam S CreditAttribution: Adam S commentedI just encountered this issue on a new install of Drupal 7.4. I'll try the patch.
Comment #52
jose.guevara CreditAttribution: jose.guevara commentedIs there a way to find out how and where it failed (logs) or do I need to setup the whole QA thing on my own to figure it out? Or is there a way for me to run my personal tests on my patch without having to re-upload it to this thread every time?
I ran a clean install of Drupal with the testing module and ran a test with the comment module and it checked out. I'm a little confused......
Thanks!
Comment #53
bfroehle CreditAttribution: bfroehle commented#48: 1015946-48.patch queued for re-testing.
Generally you cannot get more information about the failed tests beyond the short summary view. Do the tests pass locally (using the Testing module) after your patch has been applied?
Comment #55
jose.guevara CreditAttribution: jose.guevara commented@bfroehle, I only ran the comment module test. And it passed.
I'll run it all.
JG
Comment #56
jose.guevara CreditAttribution: jose.guevara commentedI've been trying to run the simpletest on my local, default, newly installed drupal. I keep getting two failed tests (before I even patched it), on drupal_render(), 'Defaults work' and 'Passing arguments to theme functions works'.
Do you guys manage to run a test that's 100% success? Or, would you have pointers (configuration, optimization, etc...) to make sure DrupalRenderTestCase->testDrupalRenderThemeArguments() passes?
Thanks,
JG
Comment #57
webengr CreditAttribution: webengr commentedI think this is message still happening for admin login after update drupal 7.4 to 7.7 also, but the site I saw it on has hordes of modules to do media gallery turned on also... BTW rebuilding permissions nor clearing cache made any difference when done before logout and log back in.
Comment #58
pioto CreditAttribution: pioto commentedsubscribe
Comment #59
guile2912 CreditAttribution: guile2912 commentedSubscribing, got the same problem on drupal 7.7
Comment #60
dddave CreditAttribution: dddave commentedGot this message for the first time when I tested my site with IE9. Curiously I cannot recreate this atm...
Could somebody summarize what we can do here atm? Are we still in "its a contrib mistake"-stage or did #48 proove this is a core bug?
Comment #61
catchMoving metadata and subscribing. There's another patch in the queue dealing with $user->cache bugs which iirc just uses the session value directly. That might be a more straightforward fix here. Can't locate it now but subscribing for later.
Comment #62
Dig1 CreditAttribution: Dig1 commentedFYI...Today I updated a site from 7.2 to 7.7. After logging off and then logging back on again '"Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem()' error message appeared.
I'd never seen it before and did not know if it was caused by a module I had just enabled. So I googled and came here. Once I could see it was core related I checked the cache status in core and noticed that, even though caching was not implemented, the following WAS showing:
- Minimum cache lifetime 12 hours
- Expiration of cached pages 12 hours
I reset both these fields back to 'none' and, after saving, the cache error message disappeared. I then tested with Cache pages for anonymous users and Cache blocks enabled and the problem did not reappear.
That fixed my temporary problem :).
Dig
Comment #63
georgemastro CreditAttribution: georgemastro commentedHad the same problem moving from 7.2 to 7.7 but it was showing up when the order was completed in Ubercart. #62 fixed the problem. Many thanks!
Comment #64
pillarsdotnet CreditAttribution: pillarsdotnet commentedWhy not just define a
user_user_load()
function that adds the$cache
property?Comment #65
bfroehle CreditAttribution: bfroehle commentedInstead perhaps we should put this in user_login_finalize() so that it only exists for logged in users?
Comment #66
pillarsdotnet CreditAttribution: pillarsdotnet commentedSure.
EDIT: Nope; it still fails tests. See [#1246796-11]
Comment #67
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe patch in #64 allows Memcache tests to complete without generating exception errors; the patch in #66 does not.
See [#1246796-12].
Comment #68
bfroehle CreditAttribution: bfroehle commentedpillarsdotnet: Can try #66 with the following additional patch to simpletest which makes it call user_login_finalize()?
Comment #69
pillarsdotnet CreditAttribution: pillarsdotnet commented(followed by a copy of the front page)
Comment #71
pillarsdotnet CreditAttribution: pillarsdotnet commentedThis patch, which is slightly less intrusive than the one in #64, also allows Memcache tests to run to completion.
Comment #73
pillarsdotnet CreditAttribution: pillarsdotnet commentedCorrected.
Comment #74
bfroehle CreditAttribution: bfroehle commentedHow about this patch which adds $user->cache in user_login_finalize() and in drupal_web_test_case.php?
Comment #75
pillarsdotnet CreditAttribution: pillarsdotnet commentedYup; that works, too.
Comment #76
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's not add another plaster on a wooden leg: the minimum cache lifetime feature is utterly broken, and I already recommended to remove it (#730060: Replace CACHE_TEMPORARY, cache_clear_all() and minimum cache lifetime with cache tags support). Don't use it. Ever.
If we want to fix it properly, we would need something among the lines of this patch:
$user->cache
should just not exist, and this information should be stored in the session itself.Comment #77
pillarsdotnet CreditAttribution: pillarsdotnet commentedThis solves the same problem as #730060: Replace CACHE_TEMPORARY, cache_clear_all() and minimum cache lifetime with cache tags support.
Either this issue or that one should be closed as a duplicate.
Comment #78
catchYeah #76 looks the right approach to me, we should never have beeen stuffing random things onto the $user object in the first place.
It'd be a small API change for D7 cache backends, as a memcache co-maintainer I would not mind that API change but if not #74 is probably still viable for D7.
Comment #79
catch#76 is not removing minimum cache lifetime, it's just using $_SESSION directly instead of putting things into session then back out again into $user->cache, which is the same as #61 suggested.
This finally reminded me what the other patch is that deals with this issue, it's #1079518: When one cache table is cleared, cached items from other tables invalidated. That fixes another bug with $user->cache and also switches to using $_SESSION directly.
Comment #80
pillarsdotnet CreditAttribution: pillarsdotnet commentedOfftopic: Would appreciate some small feedback as to whether I'm completely wasting my time at #1246796: Memcache fails its own tests.
Comment #81
Caffeine Addict CreditAttribution: Caffeine Addict commentedSubscribing - I have this issue on one of my sites on the first page after a user logs in. D7.8
Does the patch need testing?
Comment #82
killua99 CreditAttribution: killua99 commented#76 work very well for D 7.8
Comment #83
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #84
catchThis is rtbc for 8.x
For 7.x memcache is not even using $user->cache iirc, however other cache backends potentially could be. I reckon we should not immediately commit to 7.x and check what boost, apc, file cache are doing with it if anything.
Comment #85
catchComment #86
Jooblay.net CreditAttribution: Jooblay.net commentedWe are using drupal 7.8 and to many modules to list:)
This error appears on login only after disabling Page Theme [Page Theme 7.x-1.0-rc1 Allows to use different themes than the site default on specific pages.]
The error is: Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 399 of /tap/name/public_html/mysite/includes/cache.inc).
Once enabling the Page Theme the error goes away totally and we have never seen this any where else at any time.
DrupalUP Nasty
Comment #87
catch#76: 1015946-repair-minimum-cache-lifetime.patch queued for re-testing.
Comment #89
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-roll of #76. No code changes; stuff just got moved around.
Comment #90
catchMajor since a user who posts a comment can randomly invalidate expensive caches if they click around afterwards.
Comment #91
Dries CreditAttribution: Dries commentedI need to thinker about this patch some more. I want to make sure it doesn't negatively affect the cache-ability of pages with Varnish etc. After all, it looks like we'd be writing to $_SESSION more often ...
In the mean time, I have some food for thought and discussion:
The name 'cache_flush' isn't very intuitive in my opinion. I think something like 'expire' might be a bit more intuitive. Other thoughts or suggestions?
I'm not sure this check is always a good idea. It is a bit of an edge case, but switching the 'cache_lifetime' value from x to 0 would cause existing sessions to be never cleaned up.
Comment #92
catch$_SESSION['cache_flush'] - what about $_SESSION['cache_invalidate']?
This patch should not involve writing to session any more than we do now. $user->cache is stored in sessions already, but is special cased and taken from and loaded into $user->cache - this patch just uses the session API directly rather than the magic user property so should not affect anything there.
The session cleanup should definitely happen regardless of cache lifetime setting, regardless of whether that's an edge case or not it's worth doing. I moved the early return down so it's always done.
Comment #94
catchNot at system_update_8000() any more.
Comment #95
catchUpdate still had the placeholder comment in it.
Comment #96
catchTagging - since a single user posting a comment can start nuking your {cache}, {cache_bootstrap}, {cache_menu} and other expensive to build caches as they browse through the site.
Comment #96.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedIssue summary
Comment #97
pillarsdotnet CreditAttribution: pillarsdotnet commentedIt looks like you missed at least one renaming of
'cache_flush'
to'cache_invalidate'
:Should probably be:
Or better yet, to (almost) fit in 80 columns:
As an aside, I think it would be better named as 'cache_expiration' instead of 'cache_invalidate' because that would match the comments better, but please don't let my bikeshedding hold up the patch.
Comment #98
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe above expressed as a patch and interdiff.
Comment #99
pillarsdotnet CreditAttribution: pillarsdotnet commentedCNR for bot.
Comment #99.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedUpdated to reflect 'cache_flush' --> 'cache_invalidate' renaming.
Comment #100
catchThere's no reason to add an extra level of indentation/nesting to make the code fit on 80 chars. Core tends to either have long one-line if statements, or break the if statement onto multiple lines (not sure what the actual standard is), but neither of those actually change the code logic.
Comment #101
pillarsdotnet CreditAttribution: pillarsdotnet commented@catch
Well, it also breaks the long rambling (not to mention inaccurate) comment into simpler chunks that are closer to their respective implementations. I would argue that the result is more readable and more understandable than the original, but feel free to disagree and re-roll if you prefer.
I personally hate code with more than 85 or 90 characters per line. My vision isn't the greatest, and lines that wrap two or three times make reviewing patches really difficult.
Nope. Breaking a single
if
statement into multiple lines is expressly forbidden by Coding standards as modified by sun. He kept marking my code "needs work" because I like to break long conditionals, and I kept asking him to point to a coding standard that supports his opinion, until finally he changed the standard. The relevant text is:I suppose you could write something like:
or maybe even:
But the current standard expressly forbids:
True, but
Comment #102
pillarsdotnet CreditAttribution: pillarsdotnet commentedPer discussion with catch on IRC, re-rolled in two versions:
'cache_invalidate'
to'cache_expiration'
.Comment #104
catchPatch #2 looks good to me, agree with 'cache_expiration' for the session key.
Comment #105
catchWell apart from that.
Comment #106
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled.
Comment #108
pillarsdotnet CreditAttribution: pillarsdotnet commentedGrrr... Forgot to commit before re-rolling. Once again...
Comment #109
pillarsdotnet CreditAttribution: pillarsdotnet commentedCrud. I just realized that I'm mixing up #1015946 and #1272706...
EDIT: Still got the filename wrong, but the contents are right, at least.
Comment #110
pillarsdotnet CreditAttribution: pillarsdotnet commentedSince this is still marked "Needs backport", here's:
fix-per_user-cache-expiration-1015946-110-1-d7.patch
fix-per_user-cache-expiration-1015946-110-2-d7.patch
fix-per_user-cache-expiration-1015946-110.patch
Comment #111
Dries CreditAttribution: Dries commentedInstead of ignoring it, would it be appropriate to do the clean-up here? In other words, should we just add a
unset()
. Would that allow us to simplify the garbageCollector() function without affecting cache-ability/performance?Should this be 'sessions' (plural) or 'session data' instead of 'session' (singular)?
Comment #112
pillarsdotnet CreditAttribution: pillarsdotnet commentedNo; different users (different roles, actually) can have different minimum cache times.
From memory:
The admin pokes around and wants to see fresh data, but it's okay (or even preferable) if anonymous users browsing at the same time see cached data instead.
It should be 'session data', I think. But AAAARRRRGGGHHH!!!! Missed a
cache_flush
-->cache_expiration
rename.(Why did the tests pass? And how could we have properly tested for this?)
Re-rolled.
Comment #113
pillarsdotnet CreditAttribution: pillarsdotnet commentedD7 backport re-rolled.
Comment #114
pillarsdotnet CreditAttribution: pillarsdotnet commentedOr rather,
Comment #114.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedrename 'cache_invalidate' to 'cache_expiration'
Comment #115
pillarsdotnet CreditAttribution: pillarsdotnet commentedSummary updated to indicate the two possible approaches for D7.
Comment #116
catchWhen the user has $_SESSION['cache_expiration']['bin'] set, any cached items they browse are invalidated, but anything in the cache bin that they don't come into contact with is left intact. The main purpose of this that I'm aware of is for anonymous users who post comments, so they can see it come up on the page they posted them to.
With that hunk, we're not checking if the $_SESSION variable is stale, instead whether the cached data is older than the session variable, so it doesn't help the session cleanup at all.
Most of this is not really related to the patch though - we're already using the session to hold this information, just in a special cased and buggy way. This should have been updated when we moved to multiple cache bins instead of one, but no-one ever got to it.
Comment #117
catch#112: fix-per_user-cache-expiration-1015946-112.patch queued for re-testing.
Comment #118
pillarsdotnet CreditAttribution: pillarsdotnet commentedHow do you check whether the
$_SESSION
variable is stale?Comment #119
catchThat's what this hunk is doing:
Comment #120
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay, please forgive my ignorance, but I still don't "get" it.
What, in your opinion, is the difference between what the patched code is doing and what the patched code should be doing?
Comment #121
catchI think the code is fine, I was just pointing out it's necessary to have that hunk to Dries. This is rtbc as far as I'm concerned so marking as such, will give it time for someone to disagree before actually committing. On the D7 backport, memcache is already using more or less exactly the same code as introduced here so the is no issue backporting for that, however I didn't check other cache backends.
Comment #122
chx CreditAttribution: chx commentedThanks for working on this patch. It could use an automated test before it is committed. If you need help you can ask the friendly people in the #drupal-contribute IRC channel.
Comment #123
catchYeah chx is right, even in automated form.
Test for this should be doable just using the cache API - call cache_clear_all(), check that an item each from page and block cache aren't valid, check the $_SESSION variable.
Comment #124
cindyr CreditAttribution: cindyr commentedPer #114, I tried to apply this patch against current Drupal (7.8) and it didn't apply correctly, nor did it clear the error.
Comment #125
cindyr CreditAttribution: cindyr commentedI went through and manually applied the changes. It looks like the diffs didn't encompass all of the changes you had made, but using the patch file I could manually make all of them. Once updated, this does seem to have removed the problem.
Comment #126
rickmanelius CreditAttribution: rickmanelius commentedHi @cindyr. Did you apply the hunks of code in #112?
Any idea when this will commit to the D7 branch?
Edit: apologies for not focusing on the summary issue. But it does appear there is some discrepancy between the summary proposing #76 and @catch suggesting #112.
I see where D7 stand for now, and I'll assume it's not going to be ready for prime time without some additional work at this point.
Comment #127
Zachmo CreditAttribution: Zachmo commentedsubscribing
Comment #128
vallab444 CreditAttribution: vallab444 commentedHi, Balu: this works
thank you very much
-vallab
Comment #129
willkaxu CreditAttribution: willkaxu commentedSolution #13 works for me in my Drupal7.10 website. But I think the bug is just hidden in this way. Need some solution to fix the bug.
Comment #130
catchPlease don't change issue versions, see http://drupal.org/node/767608 for why this is against 8.x.
Comment #131
catchThere are already tests for this issue at #1079518: When one cache table is cleared, cached items from other tables invalidated except they don't seem to fail yet.
Comment #132
WilliamB CreditAttribution: WilliamB commentedUnrelated question, but how do i apply a patch automaticaly on a window machine?
I've been applying patch manualy up to now.
And if i desactivate the min cache feature, does it impact in anyway the way the cache is handled for anonymous users? I've had problem before i enabled cache with the server crashing because of an overload. But the user visit the site as anonymous, only the admin can log in.
thanks in advance.
Comment #133
Diogenes CreditAttribution: Diogenes commentedSo I tried to post here, clicked on the preview button, and my considered effort went to that big bit bucket in the sky. It was a closed issue, for sure, so why is there a form to post a comment?
Try again, here goes - Today I got this error:
I was messing with the "User login" block and set it to appear on a single 'login-page' page. Then I deleted the info in the textarea in the Pages subform. So now it was set to "Only the listed pages" with no pages listed (duh).
The error appeared when I logged on (via /user).
Select "All pages except those listed" - problem solved.
Comment #134
Marc Bijl CreditAttribution: Marc Bijl commentedLike said by others before (knowing it's not a solution though):
#13 works for me in a Drupal7.10 website.
Comment #135
pillarsdotnet CreditAttribution: pillarsdotnet commented#112: fix-per_user-cache-expiration-1015946-112.patch queued for re-testing.
Comment #137
rajeesh CreditAttribution: rajeesh commented#13 works for me too in a drupal 7.2 web site. Does anyone find the root cause of this problem?
Comment #138
catchPlease don't change issue status/version/priority for no reason, that massively decreases the chances of the bug getting fixed.
Comment #139
phthao CreditAttribution: phthao commented#13 Thank you!
Comment #140
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled #112
Comment #141
pillarsdotnet CreditAttribution: pillarsdotnet commentedSide note: I don't understand the "Needs tests" tag. The existing code works; the replacement code works; the only difference is that we are moving toward a more elegant and maintainable approach to doing the same thing.Retracted: See #143 below.
Comment #142
bzsim CreditAttribution: bzsim commentedI went live with my site and turned on all the caching and now when I log in I get the same error
So it looks like no one has a solution yet that actually fixes it?
Comment #143
pillarsdotnet CreditAttribution: pillarsdotnet commentedDid you apply and test the D7 patch, or are you merely complaining that it has not yet been accepted into Drupal core?
Here is a possible way to construct a test:
Authenticate as a newly-created user.
Set a cache object.
Retrieve the same cache object.
Check for a PHP warning from this line of
DatabaseBackend::prepareItem()
Comment #144
ejzambra CreditAttribution: ejzambra commentedPase del Drupal 7.10 => 7.12
Cuando iniciaba sesión con cualquier usuario me aparecia este error
Notice: Undefined property: stdClass::$cache en DrupalDatabaseCache->prepareItem() (línea 422 de /carpeta_drupal/includes/cache.inc).
He estado leyendo sus comentarios pero aun no he dado con la solución o por lo menos no les entendido entonces. Pero hice unas pruebas variando los parámetros de "Mínimo de permanencia en caché" y "Caducidad de las páginas en caché ", el resultado fue que pude por lo menos hacer que no me aparezca el error al iniciar sesión, ya solo faltaría meterse al código y repararlo. Para evitar el mensaje de error deben:
1. Poner a "Mínimo de permanencia en caché" en .
2. Poner a "Caducidad de las páginas en caché " en .
3. Hacer clic en el botón "Guardar configuración"
4. Hacer clic en el botón "Vaciar todas las cachés"
Después puedes volver a darles los valores a "Mínimo de permanencia en caché" y "Caducidad de las páginas en caché ", por lo menos eso es lo que me resulto a mí.
Saludos espero les resulte, Dios les bendiga.
Comment #145
pillarsdotnet CreditAttribution: pillarsdotnet commented(sigh)
Comment #146
pillarsdotnet CreditAttribution: pillarsdotnet commentedTried writing a test. Unfortunately, the test passes, even without the fix.
Comment #147
pillarsdotnet CreditAttribution: pillarsdotnet commentedLet's try that again. Realized that I have to set the cache item with something other than CACHE_PERMANENT.
Comment #148
pillarsdotnet CreditAttribution: pillarsdotnet commentedYay!!!! Somebody RTBC this quick so we can get it in!
Comment #149
pillarsdotnet CreditAttribution: pillarsdotnet commentedRemoving tag, since we now have a working test.
Comment #149.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedNote that there are two possible 7.x approaches.
Comment #150
chx CreditAttribution: chx commentedNice job.
Comment #151
catchI'd actually been working on tests for this today, fortunately a different test to the one pillarsdotnet made.
Here's a patch with the two tests combined - note I've had to hack $user->cache in the test case to make it fail, that obviously can't go into the final patch since that won't exist any more.
Comment #153
catchRight patch this time..
Comment #154
catchNow with the test updated to match the new code, and the fix itself. The test will pass without the $_SESSION tweaking, but it would potentially a false pass because of REQUEST_TIME - i.e. it'd pass even if the bug crept back in.
Not sure if it was previously discussed here already, but removing the db column in 8.x feels right to me, since we don't know for sure whether there are custom contrib cache handlers using it in D7 or not.
Since cache handlers are pluggable, there's no API change for Drupal 7 with this patch - anything other than a cache backend looking at $user->cache would be a bug in that module - memcache doesn't set it for example.
Only test changes here so hoping for another quick RTBC :)
Comment #155
catchgrrr, #153 was the wrong patch. It fails but not as it should have, re-upping - and the combined patch so the issue doesn't keep getting marked CNW by the bot.
Comment #156
catchbeejeebus pointed out we still have some variable_get() in the database cache backend, which should have been removed with the CMI conversion.
This doesn't get all of them, however #891600: Page and block caches are cleared the same way on cron as on content change tidies those up and adds additional test coverage. This patch just converts the one variable_get() call that was added/moved here to use config() instead.
Comment #157
catchffs.
Comment #158
Anonymous (not verified) CreditAttribution: Anonymous commentedok, this looks good to me, discussed with catch, RTBC.
Comment #159
catchI've only done re-rolls and test additions here, so I've gone ahead and committed/pushed this to 8.x.
Moving to 7.x for backport.
Comment #160
pillarsdotnet CreditAttribution: pillarsdotnet commentedBackported. I suppose that if accepted, this needs an api change node?
Comment #161
pillarsdotnet CreditAttribution: pillarsdotnet commentedWithout the column removal in modules/system/system.install this time, as per #154:
Comment #162
catchWe should probably revert the drupal_anonymous_user() and session.inc changes for D7 as well, this means we have a completely useless user->cache property, but no chance of an unexpected regression that way. Otherwise re-roll looks great though!
Comment #163
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled per #162, but seems like there ought to be a comment somewhere to the effect that
$user->cache
is now being ignored.Comment #165
pillarsdotnet CreditAttribution: pillarsdotnet commentedAnybody want to test this locally and/or hazard a guess as to why a
try { ...}
block that was not modified by this patch would suddenly start producing a fatal error?Comment #166
pillarsdotnet CreditAttribution: pillarsdotnet commentedNevermind; found it, I think.
Comment #168
pillarsdotnet CreditAttribution: pillarsdotnet commentedBackport is becoming non-trivial...
Comment #169
catchThis looks great now. I think we can put the $user->cache note in a change notice.
Comment #170
virtuali1151 CreditAttribution: virtuali1151 commented#168 worked for me...
Comment #171
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdded change notice.
Comment #172
jghyde CreditAttribution: jghyde commentedTested and deployed #168 with no problems at http://hydeinteractive.com/.
Thanks!
Joe
Comment #173
webchickCool, I had some concerns about this patch when I reviewed it back in Denver, but looks to be safe now.
Committed and pushed to 7.x. Thanks!
Comment #174
webchickThis is also worth a release notes mention.
Comment #176
c960657 CreditAttribution: c960657 commentedFYI: Support for $_SESSION['cache_expiration'][$bin] was later removed in #730060: Replace CACHE_TEMPORARY, cache_clear_all() and minimum cache lifetime with cache tags support.
Comment #176.0
c960657 CreditAttribution: c960657 commentedUpdated to point to the current patch.
Comment #177
fgmFWIW, the MongoDB cache now has a patch matching this change : #2547411: Match core flushing logic and ensure expiration always happens.