If page caching is on, and css aggregation is on:

  1. Anonymous user goes to /, front page gets cached with references to an aggregated CSS file
  2. If CSS changes and anon user goes back to front page, it still references the old CSS file because the HTML is cached. This is okay
  3. If an admin goes to admin/build/themes, system_themes gets called. The first function call is drupal_clear_css_cache() which deletes all aggregated CSS files.
  4. If an anon user goes back to /, it still references the same CSS file, but it no longer exists!
  5. Whenever the CSS cache is cleared, page cache should also be cleared.

    This patch provides that in common.inc

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

FileSize
642 bytes

Committed to 5.x. Needs to go into 6.x and HEAD.

drumm’s picture

Version: 5.7 » 6.x-dev
Robin Monks’s picture

Status: Needs review » Reviewed & tested by the community

Patch works on 7.x. RTBC!

Page cache test: 3 passes, 0 fails, 0 exceptions
Cache saving test: 5 passes, 0 fails, 0 exceptions
Cache clear test: 8 passes, 0 fails, 0 exceptions

Robin

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

This one is crying for a test.

Robin Monks’s picture

#4, Damien: I just tested for 7.x. Let's get it in there at least and then "backport" to 6.x after.

Robin

Damien Tournoud’s picture

Version: 6.x-dev » 7.x-dev
Robin Monks’s picture

Status: Needs work » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Needs review

Putting cache_clear_all() in drupal_clear_css_cache() is a bit of a hack, IMO. All of a sudden, drupal_clear_css_cache() does a ton more than it is supposed to do. Needs more discussion, and most likely needs work too.

JacobSingh’s picture

Hmm...

I had the same thought, but couldn't think of a case when this wasn't the minimum desired action.

In what case would you want to clear out the CSS without also clearing out the page cache?

Robin Monks’s picture

Perhaps this should be fixed by going through core and in every case where css is cleared, also making sure the page cache is cleared. It would be more code in the end, but would allow drupal_clear_css_cache to remain simple and dedicated to it's task.

Another alternate is doing away with drupal_clear_css_cache and only having cache_clear_all.

I'm just thinking out loud here.

Robin

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

mikeytown2’s picture

Issue tags: +DrupalWTF

cache should only be cleared if the form on that page was submitted. Boost uses this hook to clear its cache on a submit if js and/or css aggregation is on

function boost_admin_themes_submit($form, &$form_state) {
  if (variable_get('preprocess_css', FALSE)==TRUE || variable_get('preprocess_js', FALSE)==TRUE) {
    boost_cache_clear_all();
  }
}

Core should do the same; clear cache on submit.

ARray’s picture

waiting for 6.x bugfix.

mikeytown2’s picture

Priority: Normal » Critical
Issue tags: +Needs backport to D6

Remove drupal_clear_css_cache(); from http://api.drupal.org/api/function/system_themes_form/7.
Add drupal_clear_css_cache(); to http://api.drupal.org/api/function/system_themes_form_submit/7.

And problem solved, right?

najibx’s picture

waiting for 6.x bugfix ...

mikeytown2’s picture

mikeytown2’s picture

Status: Needs work » Needs review

As a side note, this issue prevented me from writing a patch sooner #475826: CVS Instructions on Project Page <- Please fix asap!

mikeytown2’s picture

FileSize
1.22 KB

drupal_clear_js_cache() & drupal_clear_css_cache() should only be called from drupal_flush_all_caches(). Right now it seems a little messy. This patch puts all calls to flushing the css/js cache through drupal_flush_all_caches().

mikeytown2’s picture

There needs to be some sort of hook/timestamp that will let modules like boost know that one aspect of the file cache has been cleared. Modules out there call drupal_clear_js_cache() #477830: Refactor local file caching and cache clear logic and there is no good way of detecting that it was flushed. Any ideas on how we can address this?

JirkaRybka’s picture

As for #18 patch - we shouldn't use drupal_flush_all_caches() this way (it was already discussed on the initial issue that added the central flushing button to the performance settings page). While it surely ensures all relevant data being flushed, it's a huge overkill too - things like rebuilding menus etc. are not needed on Themes page, and may take quite long and/or slow down large production sites. The flush all caches thing is for the button on Performance page, but otherwise we need to keep the granularity of cache flushing.

mikeytown2’s picture

How about a new function for flushing the file cache, that can be hooked; so when it's called the boost file cache gets cleared as well? #16 needs to be back ported though.

catch’s picture

Status: Needs review » Needs work

Yeah the patch as it stands is going to lead to similar complaints to what we have for admin/build/modules, needs something more specific.

mikeytown2’s picture

Title: If users goes to admin/build/themes and does not submit, anon users see cached pages with references to non-existant CSS » If users goes to admin/build/themes and does not submit, anon users see cached pages with references to non-existent CSS
FileSize
961 bytes

At least for admin/build/modules it doesn't nuke the cache on when viewing that page. Re-roll of #16

mikeytown2’s picture

Status: Needs work » Needs review

Fixed a spelling error in the title.
Opened a new issue #484524: Hook for file caches.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review

Setting to 'needs review' - testbot was broken.

alexanderpas’s picture

do we need a test for this? otherwise RTBC.

mikeytown2’s picture

Status: Needs review » Reviewed & tested by the community

I can't think of a test for this, it is a fairly straightforward bug fix.

Dries’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs work

Committed to CVS HEAD. Thanks.

mikeytown2’s picture

Status: Needs work » Needs review
FileSize
947 bytes

For 6.x

alexanderpas’s picture

Status: Needs review » Reviewed & tested by the community
EnekoAlonso-1’s picture

Well, today I have set up CacheRouter on my server with file as the cache method and it has been only a few hours until I have noticed the issue with the cached/obsolete css and js compressed filenames. As most of you are aware, it happens than when the css and/or js compressed files are regenerated, all the previously cached pages get obsolete, since the old path is no longer available.

Now, although I think adding hooks to clear the page cache when these css and js files are recreated, I think that is not the appropriate solution. Instead, I think the main issue resides in giving a new file name to these compressed files, by using an md5 string that changes every time.

Yes, I understand this is done to prevent browser cache issues, but, to tell you the truth, I'll rather have that issue than having to regenerate all cached pages on my site (currently I have 26k nodes and 500k comments with their own page).

So I have opted for patching common.inc to keep the filename of the compressed files constant over time:

// Fix on drupal_get_css
      // $filename = 'css_'. md5(serialize($types) . $query_string) .'.css';
      $filename = 'css_'. $media .'.css';  // EA 20090727 Fixed name to prevent issues with cached pages
// Fix on drupal_get_js 
    // $filename = 'js_'. md5(serialize($files) . $query_string) .'.js';
    $filename = 'js.js'; // EA 20090727 Fixed name to prevent issues with cached pages

Another solution, obviously, is to disable css and js compression, which I still would think would be better than having to expire all my cached pages.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed to Drupal 6 too, thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D6, -DrupalWTF

Automatically closed -- issue fixed for 2 weeks with no activity.