page cache might show messages

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

Rerolled to head.

It does not appear to be fixed.

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

Try two.

This adds another param to drupal_get_messages called "$clear_queue". drupal_get_messages function is only used in themes and now the page_set_cache.

I believe that this is a better way to correct the caching messages problem.

i dig this.

Mmm. drupal_set_message() is ment to be used in combination with POSTs. This shouldn't happen unless people use it with GETs.

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.

REST's wikipedia page states "POST is generally used for actions with side-effects, such as requesting the creation of a purchase order, or adding some data to a collection."

RFC 2616 (HTTP) states "In particular, the convention has been established that the GET and HEAD methods SHOULD NOT have the significance of taking an action other than retrieval."

So we certainly shouldn't be taking actions on GET, RESTful or not.

However, that doesn't mean messages won't get set on GET. The example I can think of for proper use of this would be notification of an action another user has taken, such as "... added you as a contact/friend/buddy" or "... sent you a message." I'm guessing there is some case where it would be okay for this in an anonymous user context.

I do think we should avoid PHP's sloppy comparison and use !is_array(...).

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

Ok,

I have added docummentation of the new param,

And I have corrected you !array() with count == 0

You guys can continue to discuss about allowing GETs to be cached.

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.

And about messages showing up on GETs, gentlemen, we are redirecting after form submits and that's when the messages show up.

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

That wont work, when you are not showing a page then you should keep the messages. Moving the clearing to index.php just below theme('page'), inside the same if could work. This, of course, indicates that another kind of test is also needed.

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:

drupal_set_message("See me after the redirect!");
header("Location: ". url('', array('absolute' => TRUE));
exit();

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.

Should this kind of test be in cache.test or common.test? It's kind of a mix of both...

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.

Would I be correct in thinking that this patch will cause any messages set after invocation of http://api.drupal.org/api/function/template_preprocess_page/7 (which invokes theme('status_messages')) to be discarded?

Using http://api.drupal.org/api/function/drupal_set_message/7 in a preprocess function is not ideal since the message appears a pageview "late" but it is at least *a* way of doing quick and dirty debugging. Similarly for debugging hook_exit().

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.

But doesn't this mean that any messages added subsequently to invocation of template_preprocess_page() will neither be shown on the current page (because it's too late) nor on the next pageview (because they *will* get cleared in drupal_page_footer())?

Actually there is something related here: #168909: Drupal messages (status/error) are cached along with nodes was committed to 6.x (on Feb 11, so just made it into 6.0) but never to 7.x ...

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.

Thanks for the link gpk. I'll test around with #168909: Drupal messages (status/error) are cached along with nodes...when I can. HEAD is wonky tonight and I need a break from core patching tonight.

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.

@25: Do you mean #201122: Drupal should support disabling anonymous sessions? Looks like the interaction with page caching is still up in the air.

In any case this still needs fixing in 6.x and 5.x. After a quick look at the patch committed to 6.x I reckon #168909: Drupal messages (status/error) are cached along with nodes only fixes part of the problem, for messages set by the time you reach the late page cache bootstrap phase (e.g. set during previous pageview before a redirect) - but won't help for messages set after this bootstrap phase but before theme_status_messages() gets invoked (e.g set in a menu callback/handler).

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

Thanks browser, for uploading a blank file.

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.

This solution also will allow other modules to say at any time, "hey, I don't want this request to be cached because I'm doing something different for this anonymous user" and it will be so, something I've needed in a module I worked on in 6.x.

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.

While a bit awkward, I think it's a good trick but it needs a lot of comments.

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.

Fixed an error I had using cache_clear_all() in the test, but we're still not caching a HEAD request before or after the patch...

So I figured out why HEAD requests were not being cached. It turns out that calling ob_end_flush(); during a HEAD request, current script execution terminates completely. If you turn on caching, change the following in page_set_cache:

      file_put_contents('log.txt', $_SERVER['REQUEST_METHOD'] . " before ob_end_flush()\n", FILE_APPEND);
      ob_end_flush();
      file_put_contents('log.txt', $_SERVER['REQUEST_METHOD'] . " after ob_end_flush()\n", FILE_APPEND);
      if ($cache && $data) {
        cache_set($base_root . request_uri(), $data, 'cache_page', CACHE_TEMPORARY, drupal_get_headers());
      }

... and then run the new 'Page cache test', you'll find that log.txt results in:

GET before ob_end_flush()
GET after ob_end_flush()
GET before ob_end_flush()
GET after ob_end_flush()
GET before ob_end_flush()
GET after ob_end_flush()
GET before ob_end_flush()
GET after ob_end_flush()
HEAD before ob_end_flush()

Notice there's no "HEAD after ob_end_flush()" :) This will also cause further problems because hook_exit() hooks will not fire since it is called after page_set_cache.

Under advice from the PHP manual, I think it's best to let PHP handle the automatic flushing of output buffers as it automatically does during the end of script execution. With this latest patch, there is no longer the one failure in the new page cache test.

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.

Revised, simplified tests. Also re-posting to get re-picked up by the testing bot.

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! Unrelated commits broke the patch.

Feh.

Yay! Cross post!

Heh...yay!

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?

+  // Always check the request method and that the user is logged out.
+  return $status && !$user->uid && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD');

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.

I believe this has been fixed in #477944: Fix and streamline page cache and session handling. Marking 'fixed'.

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

Double checked, and confirmed we do have tests. Good deal.