When the search form is placed in the dashboard (which is shown in the overlay) searching doesn't work

Subscribing.

// Edit

overlay_block_info_alter() seems to remove the search block when submitted. overlay_get_regions_to_render() only returns page_top.

Looks like this will happen with every form in a block being submitted.

UPDATE: Nevermind, got it. I'm going to have a look at this issue.

I was trying to replicate the bug to have a look at it, but I can't figure out how to put the search form in the dashboard. Can you do that through the interface, or did you make a custom block or something. These are the only options I get when adding things to the dashboard (screenshot included).

Then you need a fresh install.
Look at my screenshot...

This doesn't happen if the drupal_page_render() call is removed. I can't quite wrap my head around what that function is doing there, so I'm not sure if I'm removing something important. The overlay seems to function fine without it, though.

Either way, something funny is happening in overlay_render_region().

Let me know what you all think.

Output of overlay_render_region() is being matched in overlay_exit() to see if there are regions in the parent window that needs to be updated.

But it seems using drupal_page_render() multiple times isn't working correctly.

The overlay is becoming notorious for finding static caching bugs in other parts of Drupal :)

In this case it's the block module. What happened was that the block module was caching results of _block_load_blocks() which - despite the name of the function - does a lot more than just load blocks; it also allows other modules to alter them. What happened here was that the first results of that drupal_alter() were getting cached, even though the overlay module wanted to do something different with it the second time around.

In this patch, I just took the approach of removing the static cache. We might consider refactoring things to make it actually work, but given how rarely this function gets called, the overhead of maintaining a static cache doesn't seem worth it to me.

I just committed another block module patch, so I'm going to ask for a re-test. I'm in favor of this patch -- the amount of static caching we due sometimes drives me nuts.

At the same time, the way overlay detects page changes is also pretty nuts.

#9: block-static-cache-bug-687666-9.patch queued for re-testing.

Still applies :)

Hey, I'm all for ditching caches.

And yeah, I was also horrified to recently learn that the overlay rerenders the whole page in order to decide if the "parent" page needs refreshing.

And yeah, I was also horrified to recently learn that the overlay rerenders the whole page in order to decide if the "parent" page needs refreshing.

Huh? That's not what it does at all.

To be more specific:

• it only rerenders a small part of the page (typically the toolbar)
• and only on very rare occasions (typically only when someone using the overlay submits a form)

And since this usually just contains menus, and the menu system is nicely static-cached, the second time should barely involve any work at all.

The final code that does this was put together over Thanksgiving weekend because people felt bad that the overlay had been worked on very hard but was not really on a path where it would ever make sense to commit it, unless it could do something like this. The only alternative that had been proposed before then was to have form submit handlers throughout core and contrib add code like this:

if (module_exists('overlay')) {
  overlay_request_toolbar_refresh(.....);
}

In addition to being poor code separation, the problem with that is that there is no way to know when you actually need to do it. There is no way that a module, when saving data to the database, can know who else in Drupal might decide to display that data and in what context they would display it in. So this solution couldn't work...

Going forward, the only other alternative I can imagine for determining when to refresh part of the parent window would be to not try to determine it but to do it always. In other words, every time someone submits a form in the overlay, it could kick off an AJAX request to refresh the toolbar on the off chance that it was necessary. This would be simpler code (although only marginally so), but would mean an extra HTTP request, an extra Drupal bootstrap, still rendering the toolbar the first time just as often as before... and 99% of those extra page requests wouldn't serve any purpose because the HTML didn't actually change.

If anyone has better ideas, please create an issue :)

#655722: Changes made in an overlay session are not reflected when the user closes the overlay

I think I made a mistake with this patch. It's true that block_list() is only called from one other place in core, but it's called a number of times in a row, with a different region passed in each time. So there is actually some reason for a static cache - there is no need to keep rebuilding all that data for each separate region that gets passed in, since it doesn't change from region to region.

One possibility would be to keep the static cache but to explicitly clear it from inside http://api.drupal.org/api/function/block_page_build/7 (the place where the loop that calls this function initially gets kicked off), before the loop gets started.

Subscribing.
(This is the last touched critical D7 issue)

As simple as it is, it works. It feels a bit funny, because you leave the overlay when you submit the search form, but that's probably expected behaviour, and anyways, unrelated.

+++ modules/block/block.module	31 May 2010 23:52:20 -0000
@@ -242,6 +242,7 @@ function block_page_build(&$page) {
         $page[$region] = $blocks;
       }
     }
+    drupal_static_reset('block_list');
   }
   else {
     // Append region description if we are rendering the regions demo page.

its a one liner. (thought if I copied it here, it might lower the barrier to it getting a quick review)
I think to test this, try using the search form in the dashboard.

Powered by Dreditor.

Could someone confirm that block_list() doesn't actually run the same query twice with the patch applied - just install devel module, switch on query log should do it.

Also this needs a comment as to why we're adding the drupal_static_reset() here, there's no context whatsoever as to why we'd want to do that.

The patch in #19 works great...

To test I did a fresh install, created an article, ran cron, opened dashboard in overlay, searched ... worked.

Needs work per #21.

I verified that _block_load_blocks(), the function actually doing the query, only does that once per page.
Now comes with extensive docs. I think they're a bit too extensive, but it's quite hard to explain what's going with less words. Improvements, of course, welcome.

re #21:

Pretty please?

#29: blocks-static-cache-bug-687666-29.patch queued for re-testing.

+++ modules/block/block.module	8 Jun 2010 14:32:29 -0000
@@ -242,6 +242,12 @@ function block_page_build(&$page) {
+    // Clear the static cache here to allow modules to alter the blocks list
+    // differently in different contexts. For example, Overlay module only
+    // alters the block list to be shown in the overlay, not the one for the
+    // parent page. We do not clear the cache in block_list() because that is
+    // called once per region for every page view.

comment is in.

And the query is only run once, passes tests... rtbc

67 critical left. Go review some!

+    // Clear the static cache here to allow modules to alter the blocks list
+    // differently in different contexts. For example, Overlay module only
+    // alters the block list to be shown in the overlay, not the one for the
+    // parent page. 

The overlay parent window is a totally separate page from the child window, so they can't be sharing a static cache in PHP - this code comment isn't right. The overlay code for this operates entirely within the child window (i.e., within the overlay itself).

I think we might want to rewrite it to not use the overlay as an example here. We could try to explain it, but having a paragraph of description of how the overlay works living inside the block module codebase is not really my idea of a good time anyway :) Maybe we can just explain more generically why the cache needs to be cleared.

Re #33:
Sorry, for that, that was what I thought

What happened here was that the first results of that drupal_alter() were getting cached, even though the overlay module wanted to do something different with it the second time around.

from your #9 meant. I'd be happy to re-roll if you could explain what exactly IS meant by that.

No problem, yeah, this is unfortunately very tricky stuff. What actually happens is basically that on form submissions, the overlay needs to render the toolbar (separately from the rest of the page) both before and after the form submission is processed, to see if the form submit changed anything that it needs to tell the client in the parent window to update via JavaScript.

So, on those rare page loads, the order of things is:
1. render just the toolbar
2. render the overlay page and process the form submission
3. render just the toolbar

That process is what the static caching was messing up.

How does the code comment in the attached patch look?

+1. That looks great. Added bonus: I now actually understand what I coded.
It was your approach, I coded it, and now you added the comment, so I won't RTBC, but it's really just 1LOC (!), so it's very tempting...

That comment is much better, the first one nearly made my eyes roll into the back of my head.

A one-line critical bug fix which adds lots of nice comments. And less dangerous eyeball rollage from catch. What's not to love?

Committed to HEAD! Nice sleuthing!

Are we resetting cache on every page view in order to fix a bug that happens only on form submission (with some combination of overlay/toolbar/dashboard enabled)? i only read the diff. apologies if i got this wrong. It certainly seems to me from reading that comment that a module that "calls hook_page_build() more than once" can be expected to clear caches as needed. It looks like we are catering to an edge case here.

Re #39:
I'm not really qualified to answer that (especially the seconds part), but we did check (see #27) that the query that loads the blocks from the DB is only called once regardless of the cache reset. That may or may not answer your question.