Extend browser cache-busting support for all stylesheets, JavaScript, and images in default public catalogue

Bug #1681095 reported by Dan Scott on 2017-04-08
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Unassigned

Bug Description

Back in 2011 we added limited cache-busting for a few core JavaScript files in 28283b6f - but it would be really nice to be able to set cache expiry times of years in the future for all of the core JavaScript, images, stylesheets used in the public catalogues.

Dan Scott (denials) wrote :
tags: added: performance tpac
Dan Scott (denials) wrote :

Added another commit to the same branch to extend support to KPAC.

Dan Scott (denials) wrote :

Added another commit to set default cache expires settings in Apache much further out, as we can now rely on cache-busting.

Dan Scott (denials) wrote :

Rebased on current master to address a conflict with the ltr/rtl changes from LP#1681009

Dan Scott (denials) wrote :

These two commits still apply cleanly to current master, and would be a big performance win.

tags: added: pullrequest
Changed in evergreen:
milestone: none → 3.0-alpha
Changed in evergreen:
status: New → Confirmed

I tested this on a 7/18/2017 version of master, and I'm seeing something odd.

It looks like the block in header.tt2 gets called multiple times, and it doesn't handle that well.

# Browser cache-busting key
 145 # Fall back to the eg_cache_hash (set by autogen) so that we don't have to
 146 # add conditionals into the rest of the templates
 147 IF ctx.cache_key;
 148 ctx.cache_key = "?v=" _ ctx.cache_key;
 149 ELSE;
 150 ctx.cache_key = "?" _ ctx.eg_cache_hash;
 151 END;

I'm seeing that catch busted files in the beginning of the document show
  expert_row_close_btn.png?a16b97

and then further down they change to a different pattern.
  eg_tiny_logo.png?v=?v=?a16b97

Which looks to me like it would be caused by multiple calls of the mentioned block of template code. I don't think it causes anything to not work, but it might cause some unnecessary requests.

Maybe the code should also check to see if the ctx.cache_key == "?" _ ctx.eg_cache_hash before changing it?

Josh

Dan Scott (denials) wrote :

Good catch Josh; actually seeing this on our 2.12.3 server where we've applied this as well, now that you mention it! I think your suggested approach is the quickest way to the desired end (although the intention is that ctx.cache_key should only be set once; there's an underlying thinko there on my part, clearly...)

Should there also be a test for this, to look for links to css, js and image files that are not being prefixed. To make sure future code additions/changes use the correct format?

When I search through all the files in the templates/opac for lines that include an image file, but don't include the ctx.cache_key I get a few hits.

# egrep -r '\.(gif|png)' | fgrep -v ctx.cache_key
myopac/messages/list.tt2: src="[% ctx.media_prefix %]/images/question-mark.png" /></a>

parts/ebook_api/base_js.tt2:progress_icon = '<img id="ebook_avail_spinner" src="/opac/images/progressbar_green.gif" alt="' _ l("Checking availability for this item...") _ '"/>'

parts/advanced/search.tt2: <img id='search-submit-spinner' src='/opac/images/progressbar_green.gif'

parts/record/refworks.tt2: <a href="[% rw_uri %]" rel="nofollow" vocab=""><img src="/images/starz.png" alt="" />[% l('Export to RefWorks') %]</a>

Dan Scott (denials) wrote :

A test would be a very good idea!

I updated your branch at
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/stompro/lp1681095_cache_busting

I added the code to check to see if the ctx.cache_key had already been set to the ctx.eg_cache_hash before setting it.

And I added a few more image files.

I did some testing and now the repeated "v=" don't show up. And when I set a cache_key in the config.tt2, that shows up correctly also.

Josh

Galen Charlton (gmc) on 2017-08-07
Changed in evergreen:
importance: Undecided → Wishlist
assignee: nobody → Galen Charlton (gmc)
Galen Charlton (gmc) wrote :

I've pushed this to master, along with follow-ups that add release notes, fix a syntax error in one of the templates, and extend cache-busting to a couple recently-added resource.

Thanks, Dan and Josh!

Changed in evergreen:
assignee: Galen Charlton (gmc) → nobody
status: Confirmed → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers