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:

  1. Set $user->cache in both user_login_finalize() and DrupalWebTestCase::setUp().
    (See fix-per_user-cache-expiration-1015946-110-1-d7.patch.)

  2. Backport the 8.x patch.
    (See fix-per_user-cache-expiration-1015946-114-d7.patch).

    In #78, catch noted:

    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?

CommentFileSizeAuthor
#168 cache-expiration-1015946-168.patch6.5 KBpillarsdotnet
#166 cache-expiration-1015946-166.patch6.48 KBpillarsdotnet
#163 cache-expiration-1015946-163.patch6.44 KBpillarsdotnet
#161 cache-expiration-1015946-161.patch7.5 KBpillarsdotnet
#160 cache-expiration-1015946-160.patch8.77 KBpillarsdotnet
#157 1015946.patch7.98 KBcatch
#156 1015946.patch8.39 KBcatch
#155 cache_test_only.patch1.59 KBcatch
#155 cache_expiration.patch7.96 KBcatch
#154 cache_expiration.patch7.96 KBcatch
#153 cache_test_only.patch1.56 KBcatch
#151 cache_test_only.diff2.73 KBcatch
#147 fix-per_user-cache-expiration-1015946-147-testonly.patch1.54 KBpillarsdotnet
#147 fix-per_user-cache-expiration-1015946-147-test+fix.patch8.26 KBpillarsdotnet
#146 fix-per_user-cache-expiration-1015946-146-testonly.patch1.47 KBpillarsdotnet
#146 fix-per_user-cache-expiration-1015946-146-test+fix.patch8.19 KBpillarsdotnet
#140 fix-per_user-cache-expiration-1015946-140.patch7.08 KBpillarsdotnet
#112 fix-per_user-cache-expiration-1015946-112.patch6.42 KBpillarsdotnet
#114 fix-per_user-cache-expiration-1015946-114-d7.patch12.04 KBpillarsdotnet
#113 fix-per_user-cache-expiration-1015946-113-d7.patch12.03 KBpillarsdotnet
#110 fix-per_user-cache-expiration-1015946-110-1-d7.patch1.77 KBpillarsdotnet
#110 fix-per_user-cache-expiration-1015946-110-2-d7.patch12.02 KBpillarsdotnet
#110 fix-per_user-cache-expiration-1015946-110.patch6.41 KBpillarsdotnet
#109 cache_remove_bc-1272706-109.patch6.41 KBpillarsdotnet
#108 cache_remove_bc-1272706-108-2.patch6.34 KBpillarsdotnet
#106 cache_remove_bc-1272706-106-2.patch6.34 KBpillarsdotnet
#102 cache_remove_bc-1272706-102-1.patch6.03 KBpillarsdotnet
#102 cache_remove_bc-1272706-102-2.patch6.34 KBpillarsdotnet
#102 interdiff.txt1.4 KBpillarsdotnet
#98 fix-per_user-cache-expiration-1015946-98.patch6.75 KBpillarsdotnet
#98 interdiff.txt3.08 KBpillarsdotnet
#95 cache.patch4.79 KBcatch
#94 cache.patch4.85 KBcatch
#92 cache.patch4.85 KBcatch
#89 repair-minimum-cache-lifetime-1015946-89.patch5.44 KBpillarsdotnet
#76 1015946-repair-minimum-cache-lifetime.patch5.06 KBDamien Tournoud
#74 user-cache-1015946-74.patch1.21 KBbfroehle
#73 user-cache-1015946-73.patch850 bytespillarsdotnet
#71 user-cache-1015946-70.patch835 bytespillarsdotnet
#69 user-cache-1015946-69.patch1.52 KBpillarsdotnet
#68 simpletest_user_login_finalize.txt508 bytesbfroehle
#66 user_cache-1015946-65.patch891 bytespillarsdotnet
#64 user_user_load-1015946-64.patch907 bytespillarsdotnet
#48 1015946-48.patch506 bytesjose.guevara
#10 1015946-10.patch796 bytesbfroehle
#7 cache.inc_.patch443 bytesJej
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

letsrok’s picture

Ditto. Latest release 7.0.

otice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 395 of ...../includes/cache.inc).

Jej’s picture

Yep, 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).

antiorario’s picture

Mine was a clean install of RC3, now updated to 7.0.

droplet’s picture

if 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)

knarzer77’s picture

I have the same problem. I have made an upgrade from Drupal 6 on a xampp test system (php 5.3.1).

Jan

teiko’s picture

Same on my Side
Greetings

Jej’s picture

Status: Active » Needs review
FileSize
443 bytes

A patch to solve this, but needs review.

Jej

Status: Needs review » Needs work

The last submitted patch, cache.inc_.patch, failed testing.

sp_key’s picture

I 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

bfroehle’s picture

Status: Needs work » Needs review
FileSize
796 bytes

Jej: 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.

Jej’s picture

bfroehle: "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:

 Administration    Content type overview (content_type_overview)                  Module  Enabled        7.x-1.0-beta2  
 Caching           Memcache Admin (memcache_admin)                                Module  Enabled        7.x-1.0-beta3  
 Chaos tool suite  Chaos tools (ctools)                                           Module  Enabled        7.x-1.0-alpha2 
 Chaos tool suite  Custom content panes (ctools_custom_content)                   Module  Enabled        7.x-1.0-alpha2 
 Chaos tool suite  Page manager (page_manager)                                    Module  Enabled        7.x-1.0-alpha2 
 Chaos tool suite  Stylizer (stylizer)                                            Module  Enabled        7.x-1.0-alpha2 
 Chaos tool suite  Views content panes (views_content)                            Module  Enabled        7.x-1.0-alpha2 
 Core              Aggregator (aggregator)                                        Module  Enabled        7.0            
 Core              Block (block)                                                  Module  Enabled        7.0            
 Core              Color (color)                                                  Module  Enabled        7.0            
 Core              Comment (comment)                                              Module  Enabled        7.0            
 Core              Contextual links (contextual)                                  Module  Enabled        7.0            
 Core              Dashboard (dashboard)                                          Module  Enabled        7.0            
 Core              Database logging (dblog)                                       Module  Enabled        7.0            
 Core              Field (field)                                                  Module  Enabled        7.0            
 Core              Field SQL storage (field_sql_storage)                          Module  Enabled        7.0            
 Core              Field UI (field_ui)                                            Module  Enabled        7.0            
 Core              File (file)                                                    Module  Enabled        7.0            
 Core              Filter (filter)                                                Module  Enabled        7.0            
 Core              Help (help)                                                    Module  Enabled        7.0            
 Core              Image (image)                                                  Module  Enabled        7.0            
 Core              List (list)                                                    Module  Enabled        7.0            
 Core              Locale (locale)                                                Module  Enabled        7.0            
 Core              Menu (menu)                                                    Module  Enabled        7.0            
 Core              Node (node)                                                    Module  Enabled        7.0            
 Core              Number (number)                                                Module  Enabled        7.0            
 Core              Options (options)                                              Module  Enabled        7.0            
 Core              Path (path)                                                    Module  Enabled        7.0            
 Core              PHP filter (php)                                               Module  Enabled        7.0            
 Core              Search (search)                                                Module  Enabled        7.0            
 Core              Shortcut (shortcut)                                            Module  Enabled        7.0            
 Core              System (system)                                                Module  Enabled        7.0            
 Core              Taxonomy (taxonomy)                                            Module  Enabled        7.0            
 Core              Text (text)                                                    Module  Enabled        7.0            
 Core              Toolbar (toolbar)                                              Module  Enabled        7.0            
 Core              Update manager (update)                                        Module  Enabled        7.0            
 Core              User (user)                                                    Module  Enabled        7.0            
 Development       Devel (devel)                                                  Module  Enabled        7.x-1.0        
 Development       Devel node access (devel_node_access)                          Module  Enabled        7.x-1.0        
 Fields            Link (link)                                                    Module  Enabled        7.x-1.x-dev    
 Image             SmartCrop (smartcrop)                                          Module  Enabled        7.x-1.0-beta2  
 Media             IMCE (imce)                                                    Module  Enabled        7.x-1.1        
 Media             IMCE Mkdir (imce_mkdir)                                        Module  Enabled        7.x-1.0-beta2  
 Multilanguage     Locale updater (l10n_update)                                   Module  Enabled        7.x-1.0-alpha2 
 Other             Advanced help (advanced_help)                                  Module  Enabled        7.x-1.x-dev    
 Other             Backup and Migrate (backup_migrate)                            Module  Enabled        7.x-2.0        
 Other             Entity cache (entitycache)                                     Module  Enabled        7.x-1.0-beta1  
 Other             Pathauto (pathauto)                                            Module  Enabled        7.x-1.0-beta1  
 Other             Superfish (superfish)                                          Module  Enabled        7.x-1.8-alpha5 
 Other             Token (token)                                                  Module  Enabled        7.x-1.0-beta1  
 Panels            Panels (panels)                                                Module  Enabled        7.x-3.0-alpha2 
 User interface    Add another (addanother)                                       Module  Enabled        7.x-2.0-beta2  
 User interface    IMCE Wysiwyg API bridge (imce_wysiwyg)                         Module  Enabled        7.x-1.x-dev    
 User interface    Wysiwyg (wysiwyg)                                              Module  Enabled        7.x-2.0        
 Views             Views (views)                                                  Module  Enabled        7.x-3.0-alpha1 
 Views             Views exporter (views_export)                                  Module  Enabled        7.x-3.0-alpha1 
 Views             Views UI (views_ui)                                            Module  Enabled        7.x-3.0-alpha1 

Jej

vadimc’s picture

thanks, 1015946-10.patch sorted it for me

Balu Ertl’s picture

Now 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.

Jej’s picture

I confirm (#13, balu.ertl). I use D7 (updated from D7-RCx, migrated from D6)

parasolx’s picture

#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).

parasolx’s picture

Status: Needs review » Reviewed & tested by the community

patch #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.

wOOge’s picture

I 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.

mrintegrity’s picture

Category: support » bug
traceelements’s picture

#13 worked for me as well.

f0x.wls’s picture

Same here. I'm using fresh D7 and this one hits me. #13, balu.ertl method's worked for me :)
Thank you.

youropinioncounts’s picture

Ditto for #13. Fresh installation, with only the captcha and rss permissions modules installed.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

This 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.

jaxon77’s picture

I get this error when attempting to login with an email address after installing LoginToboggan

solomojo’s picture

Same 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).

solomojo’s picture

Patch found on post #10 worked for me.

Damien Tournoud’s picture

Status: Needs work » Postponed (maintainer needs more info)

Still cannot reproduce.

Anonymous’s picture

thanks, this comment helped me.

dakrcz’s picture

post #10 - works for me too

Balu Ertl’s picture

"Hiding the error under the carpet is the worse solution"

Sorry Damien, I just forgot to mention that I posted not a solution, but only a work-around for the issue.

bfroehle’s picture

@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. :)

triple5’s picture

Status: Postponed (maintainer needs more info) » Needs review

#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?"

RumpledElf’s picture

I 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.

sp_key’s picture

Any new developments on this one?
Has anyone identified the root of the problem?

kriskhaira’s picture

Is 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.

kriskhaira’s picture

I'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

kriskhaira’s picture

Ok, 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.

aldim’s picture

thanks, post #10 solved the problem.

bfroehle’s picture

Status: Needs review » Active

I'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...

This 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.

After looking into this some more, I'm tempted to mark the issue "won't fix" as well.

Screenack’s picture

I 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.

xcession’s picture

If 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.

Balu Ertl’s picture

to @xcession at #40: I spent an amount of time on testing my installed contrib (then core's too) modules. My steps were:

  1. disabling one module
  2. logging out
  3. logging in
  4. checking DB log to error message

I'm not sure is this the process that you meant or not, but I hope the following list of tested modules may helps:

  • Content Access (7.x-1.x-dev)
  • Chaos tools (7.x-1.0-beta1)
  • Mail System (7.x-2.26)
  • Mime Mail (7.x-1.x-dev)
  • Pathauto (7.x-1.0-beta1)
  • CAPTCHA (7.x-1.0-alpha3)
  • reCAPTCHA (7.x-1.7)
  • Google Analytics (7.x-1.2)
  • Wysiwyg (7.x-2.0)
  • Views (7.x-3.0-beta3)
  • Views UI (7.x-3.0-beta3)

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.

steenbob’s picture

I'm also having this problem. The only contributed modules I have in common with #41 are...

  • Chaos tools (7.x-1.0-beta1)
  • Pathauto (7.x-1.0-beta1)
  • Views (7.x-3.0-beta3)
  • Views UI (7.x-3.0-beta3)

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!

neoglez’s picture

Title: Notice "Undefined property: stdClass::$cache…" on login » "Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem()
Version: 7.0 » 7.2
rayvtirx’s picture

ive 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

Balu Ertl’s picture

I 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.

rogical’s picture

Version: 7.2 » 7.4

I'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).

damd’s picture

The patch in post #10 worked for me too. Drupal 7.4.

jose.guevara’s picture

FileSize
506 bytes

Hi 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.

bfroehle’s picture

Status: Active » Needs review

JG: 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.

Status: Needs review » Needs work

The last submitted patch, 1015946-48.patch, failed testing.

Adam S’s picture

I just encountered this issue on a new install of Drupal 7.4. I'll try the patch.

jose.guevara’s picture

Is 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!

bfroehle’s picture

Status: Needs work » Needs review

#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?

Status: Needs review » Needs work

The last submitted patch, 1015946-48.patch, failed testing.

jose.guevara’s picture

@bfroehle, I only ran the comment module test. And it passed.

I'll run it all.

JG

jose.guevara’s picture

I'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

webengr’s picture

I 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.

pioto’s picture

subscribe

guile2912’s picture

Subscribing, got the same problem on drupal 7.7

dddave’s picture

Got 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?

catch’s picture

Version: 7.4 » 8.x-dev
Component: cache system » user system
Issue tags: +Needs backport to D7

Moving 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.

Dig1’s picture

Status: Needs review » Needs work

FYI...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

georgemastro’s picture

Had 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!

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
907 bytes

Why not just define a user_user_load() function that adds the $cache property?

bfroehle’s picture

Instead perhaps we should put this in user_login_finalize() so that it only exists for logged in users?

pillarsdotnet’s picture

FileSize
891 bytes

Sure.

EDIT: Nope; it still fails tests. See [#1246796-11]

pillarsdotnet’s picture

The patch in #64 allows Memcache tests to complete without generating exception errors; the patch in #66 does not.

See [#1246796-12].

bfroehle’s picture

Status: Needs work » Needs review
FileSize
508 bytes

pillarsdotnet: Can try #66 with the following additional patch to simpletest which makes it call user_login_finalize()?

pillarsdotnet’s picture

FileSize
1.52 KB
  • Applied the attached patch to a fresh checkout of Drupal 7 and installed a test "example.com" site with the standard profile.
  • Uninstalled Overlay and Dashboard modules, just because I find them annoying.
  • Installed Testing and Memcache modules.
  • Appended the following to sites/default/settings.php:
    $conf['cache_class_cache_form'] = 'DrupalDatabaseCache';
    include_once('./includes/cache.inc');
    include_once('./sites/all/modules/memcache/memcache.inc');
    $conf['cache_default_class'] = 'MemCacheDrupal';
    
  • Navigated to admin/config/development/testing and ran the Caching tests.
  • Got the following error:
    An AJAX HTTP error occurred.
    HTTP Result Code: 200
    Debugging information follows.
    Path: /7.x/?q=batch&amp;id=5&amp;op=do
    StatusText: OK
    ResponseText:

    (followed by a copy of the front page)

  • Tried running each of the four Caching tests, one by one. After a while, each test redirected to the home page and displayed the error message "No active batch."
  • Tried running the Memcache tests, with the same results.
  • Reverted to a fresh D7 checkout and re-applied the patch from #64.
  • Navigated to admin/config/development/testing and re-ran the Caching tests.
  • Tests ran to completion with no errors.
  • Ran the Memcache tests.
  • Tests ran to completion with (only) the expected failures reported in #1246796-12: Memcache fails its own tests.

Status: Needs review » Needs work

The last submitted patch, user-cache-1015946-69.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
835 bytes

This patch, which is slightly less intrusive than the one in #64, also allows Memcache tests to run to completion.

Status: Needs review » Needs work

The last submitted patch, user-cache-1015946-70.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
850 bytes

Corrected.

bfroehle’s picture

FileSize
1.21 KB

How about this patch which adds $user->cache in user_login_finalize() and in drupal_web_test_case.php?

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community

How about this patch which adds $user->cache in user_login_finalize() and in drupal_web_test_case.php?

Yup; that works, too.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.06 KB

Let'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.

pillarsdotnet’s picture

Status: Needs review » Needs work

This 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.

catch’s picture

Status: Needs work » Needs review

Yeah #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.

catch’s picture

#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.

pillarsdotnet’s picture

It'd be a small API change for D7 cache backends, as a memcache co-maintainer I would not mind that API change

Offtopic: Would appreciate some small feedback as to whether I'm completely wasting my time at #1246796: Memcache fails its own tests.

Caffeine Addict’s picture

Subscribing - 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?

killua99’s picture

#76 work very well for D 7.8

pillarsdotnet’s picture

Title: "Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() » Eliminate $user->cache and {session}.cache in favor of $_SESSION['cache_flush'][$bin]
catch’s picture

This 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.

catch’s picture

Status: Needs review » Reviewed & tested by the community
Jooblay.net’s picture

We 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

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 1015946-repair-minimum-cache-lifetime.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
5.44 KB

Re-roll of #76. No code changes; stuff just got moved around.

catch’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Major since a user who posts a comment can randomly invalidate expensive caches if they click around afterwards.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I 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:

+++ b/includes/cache.inc
@@ -491,11 +491,10 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+      // We store the time in the current user's session. We then simulate
+      // that the cache was flushed for this user by not returning cached
+      // data that was cached before the timestamp.
+      $_SESSION['cache_flush'][$this->bin] = REQUEST_TIME;

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?

+++ b/includes/cache.inc
@@ -522,7 +521,25 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+    // Garbage collection is only necessary when enforcing a minimum cache lifetime.
+    $cache_lifetime = variable_get('cache_lifetime', 0);
+    if (!$cache_lifetime) {
+      return;
+    }

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.

catch’s picture

Status: Needs work » Needs review
FileSize
4.85 KB

$_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.

Status: Needs review » Needs work

The last submitted patch, cache.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
4.85 KB

Not at system_update_8000() any more.

catch’s picture

FileSize
4.79 KB

Update still had the placeholder comment in it.

catch’s picture

Issue tags: +Performance

Tagging - 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.

pillarsdotnet’s picture

Issue summary: View changes

Issue summary

pillarsdotnet’s picture

Status: Needs review » Needs work

It looks like you missed at least one renaming of 'cache_flush' to 'cache_invalidate':

--- a/includes/cache.inc
+++ b/includes/cache.inc
@@ -428,1 +428,1 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
-    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && $user->cache > $cache->created) {
+    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && isset($_SESSION['cache_invalidate'][$this->bin]) && $_SESSION['cache_flush'][$this->bin] > $cache->created) {

Should probably be:

--- a/includes/cache.inc
+++ b/includes/cache.inc
@@ -428,1 +428,1 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
-    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && $user->cache > $cache->created) {
+    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && isset($_SESSION['cache_invalidate'][$this->bin]) && $_SESSION['cache_invalidate'][$this->bin] > $cache->created) {

Or better yet, to (almost) fit in 80 columns:

--- a/includes/cache.inc
+++ b/includes/cache.inc
@@ -419,17 +419,24 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
     if (!isset($cache->data)) {
       return FALSE;
     }
-    // If enforcing a minimum cache lifetime, validate that the data is
-    // currently valid for this user before we return it by making sure the cache
-    // entry was created before the timestamp in the current session's cache
-    // timer. The cache variable is loaded into the $user object by _drupal_session_read()
-    // in session.inc. If the data is permanent or we're not enforcing a minimum
-    // cache lifetime always return the cached data.
-    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && $user->cache > $cache->created) {
-      // This cache data is too old and thus not valid for us, ignore it.
-      return FALSE;
+    // Check whether the cache data is temporary and whether a minimum cache
+    // lifetime is being enforced.
+    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0)) {
+      // Check whether DrupalDatabaseCache::expire() stored an expiration
+      // timestamp in the user session data for this bin.
+      if (isset($_SESSION['cache_invalidate'][$this->bin])) {
+        // Check whether the timestamp is newer than the cache entry.
+        if ($_SESSION['cache_invalidate'][$this->bin] > $cache->created) {
+          // We simulate per-user cache expiration by not returning cached          
+          // objects that were created before the cache_invalidate timestamp         
+          // stored in the user session data. This cache data is too old, so we      
+          // ignore it.                                                              
+          return FALSE;
+        }
+      }
     }
-
+    // The data is either permanent or not subject to expiration rules, so
+    // unserialize and return it.
     if ($cache->serialized) {
       $cache->data = unserialize($cache->data);
     }

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.

pillarsdotnet’s picture

Title: Eliminate $user->cache and {session}.cache in favor of $_SESSION['cache_flush'][$bin] » Eliminate $user->cache and {session}.cache in favor of $_SESSION['cache_expiration'][$bin]
FileSize
3.08 KB
6.75 KB

The above expressed as a patch and interdiff.

pillarsdotnet’s picture

Status: Needs work » Needs review

CNR for bot.

pillarsdotnet’s picture

Issue summary: View changes

Updated to reflect 'cache_flush' --> 'cache_invalidate' renaming.

catch’s picture

Status: Needs review » Needs work
+    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0)) {
+      // Check whether DrupalDatabaseCache::expire() stored an expiration
+      // timestamp in the user session data for this bin.
+      if (isset($_SESSION['cache_expiration'][$this->bin])) {

There'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.

pillarsdotnet’s picture

@catch

There's no reason to add an extra level of indentation/nesting to make the code fit on 80 chars.

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.

Core tends to ... break the if statement onto multiple lines ...

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:

Conditions should not be wrapped into multiple lines.

I suppose you could write something like:

  if
    (condition1 && condition2) {
    do_something;
  }

or maybe even:

  if (
    condition1 && condition2
  ) {
    do_something;
  }

But the current standard expressly forbids:

  if (condition1
    && condition2) {
    do_something;
  }
neither of those actually change the code logic.

True, but

  1. There was an error in code logic. See the top of #97.
  2. I don't see why you would prefer a single, long, complicated, hard-to-understand line to several short, simple, easy-to-understand ones.
  3. The only standards-compliant alternative to nested conditionals is to pre-assign the sub-conditions to tersely-named temporary variables, which I find even to be even uglier as it precludes short-circuit evaluation.
  4. Since it doesn't change the code logic, why is this issue marked needs work?
pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
6.34 KB
6.03 KB

Per discussion with catch on IRC, re-rolled in two versions:

    • Corrects the logic as described at the top of #97
    • Renames 'cache_invalidate' to 'cache_expiration'.
    • Wraps a long comment that extended beyond 80 chars.
    • Removes an incorrect statement: The cache variable is loaded into the $user object by _drupal_session_read() in session.inc.
    • Generally improves the grammar and readability of the remaining portions of the same comment.
  1. An interdiff between the two.

Status: Needs review » Needs work

The last submitted patch, cache_remove_bc-1272706-102-2.patch, failed testing.

catch’s picture

Status: Needs work » Needs review

Patch #2 looks good to me, agree with 'cache_expiration' for the session key.

catch’s picture

Status: Needs review » Needs work

Well apart from that.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
6.34 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, cache_remove_bc-1272706-106-2.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
6.34 KB

Grrr... Forgot to commit before re-rolling. Once again...

pillarsdotnet’s picture

Crud. I just realized that I'm mixing up #1015946 and #1272706...

EDIT: Still got the filename wrong, but the contents are right, at least.

pillarsdotnet’s picture

Since this is still marked "Needs backport", here's:

  1. The work-around patch in #74, re-rolled against today's 7.x checkout:
    fix-per_user-cache-expiration-1015946-110-1-d7.patch
  2. The patch from #109, backported to 7.x, and combined with some trivial cleanups:
    fix-per_user-cache-expiration-1015946-110-2-d7.patch
  3. The patch from #109, appropriately renamed for this issue:
    fix-per_user-cache-expiration-1015946-110.patch
Dries’s picture

Status: Needs review » Needs work
+++ b/includes/cache.inc
@@ -462,17 +462,15 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+    if ($cache->expire != CACHE_PERMANENT && variable_get('cache_lifetime', 0) && isset($_SESSION['cache_expiration'][$this->bin]) && $_SESSION['cache_expiration'][$this->bin] > $cache->created) {
+      // This cached data is too old; ignore it.
        return FALSE;

Instead 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?

+++ b/includes/cache.inc
@@ -565,8 +562,28 @@ class DrupalDatabaseCache implements DrupalCacheInterface {
+    // handler can properly clean-up the session for anonymous users.

Should this be 'sessions' (plural) or 'session data' instead of 'session' (singular)?

pillarsdotnet’s picture

Instead 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?

No; different users (different roles, actually) can have different minimum cache times.

From memory:

We simulate a per-user minimum cache time by recording a cache_expiration timestamp in the user session and simply ignoring cache data that is older than the timestamp.

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.

Should this be 'sessions' (plural) or 'session data' instead of 'session' (singular)?


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.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
12.03 KB

D7 backport re-rolled.

pillarsdotnet’s picture

pillarsdotnet’s picture

Issue summary: View changes

rename 'cache_invalidate' to 'cache_expiration'

pillarsdotnet’s picture

Summary updated to indicate the two possible approaches for D7.

catch’s picture

When 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.

catch’s picture

pillarsdotnet’s picture

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.

How do you check whether the $_SESSION variable is stale?

catch’s picture

That's what this hunk is doing:

+    if (isset($_SESSION['cache_expiration'])) {
+      $expire = REQUEST_TIME - $cache_lifetime;
+      foreach ($_SESSION['cache_expiration'] as $bin => $timestamp) {
+        if ($timestamp < $expire) {
+          unset($_SESSION['cache_expiration'][$bin]);
+        }
+      }
+      if (!$_SESSION['cache_expiration']) {
+        unset($_SESSION['cache_expiration']);
+      }
+    }
+
pillarsdotnet’s picture

Okay, 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?

catch’s picture

Component: user system » cache system
Status: Needs review » Reviewed & tested by the community

I 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.

chx’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks 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.

catch’s picture

Yeah 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.

cindyr’s picture

Per #114, I tried to apply this patch against current Drupal (7.8) and it didn't apply correctly, nor did it clear the error.

cindyr’s picture

I 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.

rickmanelius’s picture

Hi @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.

Zachmo’s picture

subscribing

vallab444’s picture

Hi, Balu: this works

thank you very much

-vallab

willkaxu’s picture

Version: 8.x-dev » 7.10

Solution #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.

catch’s picture

Version: 7.10 » 8.x-dev

Please don't change issue versions, see http://drupal.org/node/767608 for why this is against 8.x.

catch’s picture

There 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.

WilliamB’s picture

Unrelated 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.

Diogenes’s picture

So 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:

Notice: Undefined property: stdClass::$cache in DrupalDatabaseCache->prepareItem() (line 399 of D:\htdocs\drupal7\includes\cache.inc).

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.

Marc Bijl’s picture

Like said by others before (knowing it's not a solution though):
#13 works for me in a Drupal7.10 website.

pillarsdotnet’s picture

Status: Needs work » Needs review
Issue tags: -Performance, -Needs tests, -Needs backport to D7

Status: Needs review » Needs work
Issue tags: +Performance, +Needs tests, +Needs backport to D7

The last submitted patch, fix-per_user-cache-expiration-1015946-112.patch, failed testing.

rajeesh’s picture

Version: 8.x-dev » 7.2
Category: bug » feature
Priority: Major » Normal

#13 works for me too in a drupal 7.2 web site. Does anyone find the root cause of this problem?

catch’s picture

Version: 7.2 » 8.x-dev
Category: feature » bug
Priority: Normal » Major

Please don't change issue status/version/priority for no reason, that massively decreases the chances of the bug getting fixed.

phthao’s picture

#13 Thank you!

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
7.08 KB

Re-rolled #112

pillarsdotnet’s picture

Side 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.

bzsim’s picture

I went live with my site and turned on all the caching and now when I log in I get the same error

Undefined property: stdClass::$cache

So it looks like no one has a solution yet that actually fixes it?

pillarsdotnet’s picture

@bzsim in #142:

Did you apply and test the D7 patch, or are you merely complaining that it has not yet been accepted into Drupal core?

@Needs tests tag in #122:

Here is a possible way to construct a test:

  1. Authenticate as a newly-created user.

  2. Set a cache object.

  3. Retrieve the same cache object.

  4. Check for a PHP warning from this line of DatabaseBackend::prepareItem()

ejzambra’s picture

Version: 8.x-dev » 7.12
Assigned: Unassigned » ejzambra

Pase 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.

pillarsdotnet’s picture

Version: 7.12 » 8.x-dev
Assigned: ejzambra » Unassigned

(sigh)

pillarsdotnet’s picture

Tried writing a test. Unfortunately, the test passes, even without the fix.

pillarsdotnet’s picture

Let's try that again. Realized that I have to set the cache item with something other than CACHE_PERMANENT.

pillarsdotnet’s picture

Yay!!!! Somebody RTBC this quick so we can get it in!

pillarsdotnet’s picture

Issue tags: -Needs tests

Removing tag, since we now have a working test.

pillarsdotnet’s picture

Issue summary: View changes

Note that there are two possible 7.x approaches.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Nice job.

catch’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.73 KB

I'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.

Status: Needs review » Needs work

The last submitted patch, cache_test_only.diff, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
1.56 KB

Right patch this time..

catch’s picture

FileSize
7.96 KB

Now 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 :)

catch’s picture

grrr, #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.

catch’s picture

FileSize
8.39 KB

beejeebus 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.

catch’s picture

FileSize
7.98 KB

ffs.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

ok, this looks good to me, discussed with catch, RTBC.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I'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.

pillarsdotnet’s picture

Status: Patch (to be ported) » Needs review
FileSize
8.77 KB

Backported. I suppose that if accepted, this needs an api change node?

pillarsdotnet’s picture

Without the column removal in modules/system/system.install this time, as per #154:

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.

catch’s picture

We 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!

pillarsdotnet’s picture

Re-rolled per #162, but seems like there ought to be a comment somewhere to the effect that $user->cache is now being ignored.

Status: Needs review » Needs work

The last submitted patch, cache-expiration-1015946-163.patch, failed testing.

pillarsdotnet’s picture

Anybody 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?

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
6.48 KB

Nevermind; found it, I think.

Status: Needs review » Needs work

The last submitted patch, cache-expiration-1015946-166.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
6.5 KB

Backport is becoming non-trivial...

catch’s picture

Status: Needs review » Reviewed & tested by the community

This looks great now. I think we can put the $user->cache note in a change notice.

virtuali1151’s picture

#168 worked for me...

pillarsdotnet’s picture

jghyde’s picture

Tested and deployed #168 with no problems at http://hydeinteractive.com/.

Thanks!

Joe

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, 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!

webchick’s picture

Issue tags: +7.13 release notes

This is also worth a release notes mention.

Status: Fixed » Closed (fixed)

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

c960657’s picture

FYI: 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.

c960657’s picture

Issue summary: View changes

Updated to point to the current patch.

fgm’s picture

FWIW, the MongoDB cache now has a patch matching this change : #2547411: Match core flushing logic and ensure expiration always happens.