page cache might show messages

i'd like to know if this happens with typical php.ini settings.

drupal_get_messages is not patched and I am not 100% patching it in this way is the proper solution.

i dig this.

is there some reason why we can't allow drupal_set_message() on GET? there are all sorts of GET urls than can conceivably take actions. think REST APIs. I don't like this limitation.

... /and/ this new parameter should be documented.

I was suprised to see that count(NULL), which is what I think the no messages case would be, doesn't seem to be throwing any errors and count() actually is documented as supporting NULL.

Committed to HEAD.

Balazs Nagykekesi reported today morning that this fix does not work (no longer? never worked?) because theme_status_messages clear the messages before this gets a chance to fire. Dave Reid and msyelf confirmed this broken and Dave works on the patch.

Edit: for sure it's broken in D5, D6, D7.

Initial patch that does not change any APIs, so will be safe to backport to 6.x and 5.x. WITH A TEST!

I tested this on forms that have a #redirect set, and it worked fine, I saw the message I was supposed to see. I even tested it with using:

and I saw a message after a redirect. This seems to be working just fine. The only case it doesn't work is if you use the location header without an exit() right after, which is not recommended by the PHP manual, and is not used at all throughout core.

Revised test that checks all the conditions for not caching a page. I've confirmed that with just the test in the patch applied, it fails. With the entire patch, it passes.

No, the patch changes the call to drupal_get_messges() in template_preprocess_page() to not clear out the messages. Only when code execution hits drupal_page_footer() (one of the last calls in index.php) are the messages cleared.

So my hunch is that the latest patch at #168909 fixes this for 7.x, and that 6.x is fixed already (I've not tested this yet). #168909 does need tests, which is why it was never committed.

5.x is another matter.

Wouldn't this get fixed by the patch that reworks the session handling so we only write to the database when a session variable is set? Let's give priority to that other patch.

I've been racking this for a while, and I think I've figured out an even better end-all solution. This patch adds a new global $cache_page. Any time before page_set_cache() it can be set to FALSE at any time to not cache the page, i.e. drupal_set_message(). So now it doesn't matter if the messages a cleared in theme_status_messages(). Plus, any messages set after theme_status_messages() will show up on the next request.

EDIT: This is not the patch you're looking for... (see patch in #29 down below).

Ok, there we go. Attached is the patch described in #27 along with a test for said patch testing all the conditions that should NOT get a page cached.

Here's also a first stab at not introducing a new global by implementing it into page_get_cache. This could be done better, but right now, I would like validation that this is an acceptable solution.

After discussion with chx, I added a new function, page_cache_allowed() which retrieves and optionally sets the current cache status for the page. During testing, I noticed that HEAD requests were not cached as supposed to with the patch, but when I ran the same added test from the patch, currently we don't cache HEAD requests anyway, so I'm investigating it.

The last submitted patch failed testing.

Since this seems to be the only viable option to fix the caching bug, I wonder if this patch could get a go-ahead to be backported to D6 as well.

Here's a D6 backport that doesn't break any current APIs, but just introduces the new function page_cache_allowed().

@Dave -- I have merged in the changes from #322104: Allow hook_boot() to disable page cache to this patch and extended the test case.

Yay! Cross post!

Re-rolled for changes from #201122: Drupal should support disabling anonymous sessions and changing up the tests to improve coverage. As of now, getting one expected fail because hook_exit is not firing for HEAD requests. Should not pass the bot.

page_cache_allowed(FALSE) seems useful. However, calling page_cache_allowed(TRUE) may revert a previous decision about not caching the current page. This may be dangerous, if the caller is not very careful. The caller generally does not know what caused caching to be disabled for the current request. I wonder under what circumstances you would like to cache a page that was previously marked as uncacheable?

The request method doesn't change during a request, so the check for GET/HEAD may be moved into the if (!isset($status)) { block earlier in the function.

Now that the logic about whether to cache or not is moved to page_cache_allowed(), it may make the purpose of page_get_cache() clearer, if the call to page_cache_allowed() from page_get_cache() is removed, and callers instead call page_cache_allowed() prior to calling page_get_cache() when appropriate.

Note that over in #201122: Drupal should support disabling anonymous sessions it is being discussed whether output buffering should always be enabled. This might make it easier to solve the issue with HEAD requests.

Have we added tests to ensure messages are not shown in cached pages?