Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Working on tests for cacherouter I found that sometimes $user->cache is not initialized for example in case fast-page-cache
here is a simple fix
Comment | File | Size | Author |
---|---|---|---|
#13 | 698248-cache-notice-d7.patch | 949 bytes | andypost |
cache_notice.patch | 714 bytes | andypost | |
Comments
Comment #1
andypostD7 version is different but notice in in caching is really bad, tagged
Comment #2
andypostMost of time $user->cache is not initialized so it seems isset() check is less resource intensive then current implementation
Comment #3
droplet CreditAttribution: droplet commentedComment #5
Gábor HojtsyWhy not couple that into a !empty($user->cache)?
Comment #6
andypost@Gábor Hojtsy, just because isset() faster. We just need to check if this property set
Comment #7
Gábor HojtsyIf you just need to see if it is set, why do you have "&& $user->cache" after that?
Comment #8
andypostbecause I need to check if this property isset() and if so to continue comparison
Comment #9
Gábor HojtsyWhere does core use or our coding guidelines suggest this pattern to "improve performance"?
Comment #10
andypostAs I posted in original issue - $user->cache is not avaliable in page_cache_fastpath()
Edit So this lead to notices in bootstrap when using custom cache implementation
Comment #11
Gábor HojtsyOk, misread the patch, sorry. Committed!
Comment #12
andypostI found that D7 affected too, but not sure about page_cache_fastpath() in D7 so need review
Comment #13
andypostPatch
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedI don't think would be an issue any more, I couldn't find the code patched in #13 at all. I guess it's been refactored significantly since. The closest I could find was:
Which looks like it already has an isset() check, albeit for a different variable.
If this is a bug report for D7, we should write a test to show that it exists, but I think maybe we should just confirm manually that it does before going that far.
#2094585: [policy, no patch] Core review bonus for #2025699: Consolidate 'datetime'/'datetime_form' and 'datelist'/'datelist_form' #type and #theme names for consistency.
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedTagging for an issue summary update so we can get clear steps to reproduce the issue.