Jump to:
| Project: | Panels |
| Version: | 5.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | dww |
| Status: | closed (won't fix) |
Issue Summary
When you submit a node edit, core calls cache_clear_all(), since the page cache for that node need to be cleared, along with any other page on the site that might include a listing that references the node. It'd be way more costly to try to selectively clear just the pages that need to, so core clears the whole page cache.
Seems like an oversight that when you edit a panel (certainly a panel_page, and probably a mini panel, hence component "Code"), the page cache isn't cleared. If I change something about a given display, the next time an anonymous user visits that path, they should get the new version, since the cached copy is now invalid.
Here's a trivial (untested) patch to add a cache_clear_all() at the end of panels_common_cache_clear(). However, I don't fully grok all of panels' own internal caching behavior, so I'm not sure if that cache_clear_all() would break things that panels is assuming/relying on. But, it should be safe, since cache_clear_all() with no args just clears the entire {cache_page} table but nothing else...
| Attachment | Size |
|---|---|
| panels_clear_page_cache.patch | 600 bytes |
Comments
#1
Ok, I just tested this:
A) Confirmed the bug -- I turned on the page cache on my test site. Loaded a panel page as anonymous, in another browser, edited the content on the panel page, reloaded as anonymous, and saw the stale display.
B) Applied the patch, tried again, and after editing the content of the panel page, anonymous saw the valid display.
I haven't tested with mini panels or anything else, but at least as far as panel_pages is concerned this is RTBC.
Thanks,
-Derek
#2
Nice catch, dww, and thanks for the patch. There are some small potential performance issues with this - because panels_common_cache_clear() is called BOTH when editing of a display is cancelled and when it is confirmed, it means that we'd be calling cache_clear_all() both in circumstances where there is new content to be seen, and where there isn't. For most smaller sites, it wouldn't be an issue; however, I'm thinking of my own use case where an enormous number of people could be editing (and likely cancelling) those edits to displays at any given time.
Maybe I'm being paranoid (I tend to be when it comes to performanec issues), I'd at least like to not force a full regeneration of the cache whenever one of those edits gets made. So I'm going to see if I can find a way to rework this that either only calls cache_clear_all() when the display has actually been saved, or if we can selectively clear the cache only when it's absolutely necessary. (That is, if you don't beat me to it in ALL the spare time you have ;) ) Of course, if it it's infeasible in core...
I'm a real caching nooblet, so I don't know - is the issue with selectively clearing the cache in core tied only to the generation of path aliases and the like, or does it go deeper than that? If it's just paths, then we can be more selective here, as there's only one, specific place that paths can be changed for a given display, and it's entirely separate from editing the content. If there's an issue having to do with the content itself, however, that could be another story.
In any case, I'm going to apply your patch to my -dev sandbox and keep it running while I work on other stuff to see if any potential problems crop up.
#3
sub
#4
I think this is still relevant.
'drush cache clear' has become a constant for me after editing panels.
#5
Perhaps, but you changed the version without providing an alternative patch. That can only cause confusion.
#6
Sorry, I thought there was some policy about fixing bugs in the current version and then backporting. I was just confirming that it also didn't work in 6.x. My mistake.
#7
With the release of Drupal 7, Drupal 5 is no longer supported.