The logic for the shopping cart for anon users fails. This is because the check for !$product_count happens in one instance, but not another.
The display logic should be:
// Build the block array for display based on cache settings.
if ($cachable && !$product_count) {
// Caching is turned on and the user is not logged in, so we should
// deliver a block that is safe for caching.
$block = array(
'subject' => theme('uc_cart_block_title', $title, $icon_class, $collapsible),
'content' => theme('uc_cart_block_content_cachable'),
);
}
Alternately, $cachable should be reset based on $product_count.
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | 1167276-reroll.patch | 1.05 KB | tr |
| #26 | uc_cart.module.rej_.txt | 1.31 KB | mrP |
| #18 | 1167276-anon-pressflow-uc-block-2.patch | 1.1 KB | agentrickard |
| #13 | 1167276-anon-pressflow-uc-block.patch | 1.05 KB | agentrickard |
| #4 | Cart-Woes.png | 537.46 KB | agentrickard |
Comments
Comment #1
longwaveWe must output the cache-safe cart block if $cachable is TRUE, no matter whether any products are in the cart. Otherwise, this can result in "ghost carts" being stored in the Drupal page cache, where subsequent users may see the cart contents of a previous user. Unless I'm missing something?
Comment #2
agentrickardWhat I don't understand is that I get different output of this block for anon users in different contexts. Even if they have items in the cart, they get served the cached block (e.g. 'View your cart') in _some_ cases but not others (when they see an itemized cart block).
Isn't there session handling involved in the $product_count logic that accounts for this?
Comment #3
longwaveI am not sure this has anything to do with sessions, $product_count is simply the number of items in the cart.
Perhaps the global $conf['cache'] is being overridden by another module sometimes, to avoid caching particular pages, so the variable_get() test is fooled? Can you identify particular cases where the block is shown to anon users?
Comment #4
agentrickarduc_cart_get_contents() depends on uc_cart_get_id(), which sets and reads a unique session variable to distinguish between users (including anonymous users).
We're not doing anything odd to the cache. Settings are:
-- Page cache: Normal
-- Block cache: Off
The problem isn't that the block is _shown_ to anon users. It's that the block is not shown _consistently_ to anon users, as the doc attached tries to explain.
I suppose it is possible that Views is interfering in some odd way, since /store is a View path and may be confusing core cache because of how menu items are handled. But I doubt that.
Comment #5
longwaveThis is a problem, when page caching is enabled. Scenario:
1. The page cache is cleared.
2. An anonymous user adds an item to their cart.
3. The same user views an uncached (but cachable) page. The page is cached, including their cart block.
4. Another anonymous user (with an empty, or at least different, cart) visits the same page, and is shown the cached page, which contains the previous user's cart block.
The cache-safe cart block exists to avoid this "ghost cart" problem; anonymous users should always see "View your shopping cart" when page caching is enabled. But this does not explain the inconsistency in your example!
If you do want to show the cart contents block to anonymous users, you must either disable the page cache, or use the Ajax cart module to replace the cart block after the page is loaded. Back in D5, Ubercart used cache_clear_all() in several places to avoid this, but as it is a huge performance hit it was removed; see #309394: Excessive use of cache_clear_all().
#449122: Hide block if cart is empty. is related, though somewhat confusing to follow.
Comment #6
agentrickardI'm thinking this might be a Pressflow variant issue. Perhaps Pressflow is disabling the page cache when $SESSION is not empty.
Investigating.
Comment #7
longwavePressflow was mentioned in #377798: Cart block always starts a session
Comment #8
agentrickardRight, so in the Pressflow world, it appears that drupal_page_is_cacheable() returns FALSE to anon users because Ubercart sets the cart $_SESSION variable.
That means that the page isn't being served from cache at all, which explains the difference.
Changing title to reflect that. Moving to 'support' request, since this can't be considered an UC bug.
[EDIT]: drupal_page_is_cacheable() is a Pressflow-specific function.
Comment #9
agentrickardI wonder if anon users should use a cookie rather than a session_id for cart identification here, since cookies wouldn't break the behavior?
We could set the cookie IFF an item is in the cart.
Testing AJAX Cart.
Comment #10
agentrickardLooks like AJAX Cart solves this problem. Marking as duplicate.
@longwave Thanks for the support!
Comment #11
longwaveI wonder if the patch from #377798-47: Cart block always starts a session will help here, without the need for Ajax cart?
Comment #12
agentrickardLet me try that.
Comment #13
agentrickardThis version works as expected. I'm not sure why the second IF had !$cacheable as the first check. If empty we should always hide.
Comment #14
longwaveGood news, thanks for testing. Before committing I would like to test this myself, and would also like some comments above this new code to explain that it's for Pressflow support.
I think the second part should be left as-is, or needs a separate Pressflow workaround, again because of the ghost cart block problem. If we change the code as you suggest, it looks like that if all the following are true:
- site is not using Pressflow
- page caching is enabled
- user is anonymous
- "hide empty cart block" is enabled
- user has items in their cart
- user visits a previously uncached page
Then later, another anonymous user with an empty cart views the same page, the cache safe cart block will be displayed?
Comment #15
agentrickardThat makes sense to me. You need a revised patch?
Comment #17
longwaveIf you have time, please do - not sure when I will get around to testing a Pressflow-enabled install, and any help here is welcome! There is also no need for the
$cachable = TRUE;line in the patch.Comment #18
agentrickardThis just adds a logic check for $is_pressflow to the current patch.
Comment #20
wxman commentedI just tried this patch to see if it would get rid of the cart showing on all anon visitors. It works perfectly on my Pressflow install. The only weird thing is my copy didn't have
$uc_cart_path = drupal_get_path('module', 'uc_cart');Comment #21
agentrickardIgnoring the testbot fail and marking NR.
Comment #22
mrP commented#10 http://drupal.org/node/1167276#comment-4519590 (uc_ajax_cart) is a great workaround for pressflow + anonymous checkout issues. Thanks!
Comment #23
mrP commentedFYI, confirmed in UC 2.6 / Pressflow 6.22 as well.
Comment #24
tr commented@pricejn2: So did you test the patch in #18?
Comment #25
mrP commentedSorry no. I haven't tested the patch yet. Will get around to is ASAP.
Comment #26
mrP commentedI just tried the patch and it failed out. I don't have to patch very often so it could definitely be user error. Nonetheless, the .rej file is attached.
Comment #27
tr commentedHere's a re-roll of the patch from #18 against the current 6.x-2.x-dev.
Comment #29
tr commentedLets try against HEAD, where this belongs.
Comment #30
tr commentedComment #31
tr commented#27: 1167276-reroll.patch queued for re-testing.
Comment #32
tr commentedSo the testbot likes the new patch, but that's only the first step. Now I need to know:
1) Does it fix the problem with Pressflow? Should be checked for both anonymous and authenticated cart blocks.
2) Does a non-Pressflow site still work properly with this patch? Again, should be checked for anonymous and authenticated.
Comment #33
Poieo commentedI can confirm this is working with Pressflow for anon and auth users.
I don't have a non-pressflow site to test with.
Comment #34
Stealth commented#27: 1167276-reroll.patch queued for re-testing.
Comment #35
tr commentedComment #36
lliss commentedI have tested this both on pressflow and on a fresh install of drupal 6.28. The pressflow version allows the cart to display with the number of items in the cart with caching enabled. The standard installation works exactly as it does without the patch. So I think it is safe to move forward with integrating this into the next release.
Comment #37
mrP commented+1 RTBC
Comment #38
longwaveCommitted #27.