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

Bug #1681095 reported by Dan Scott
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
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.

Revision history for this message
Dan Scott (denials) wrote :
tags: added: performance tpac
Revision history for this message
Dan Scott (denials) wrote :

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

Revision history for this message
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.

Revision history for this message
Dan Scott (denials) wrote :

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

Revision history for this message
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
Revision history for this message
Josh Stompro (u-launchpad-stompro-org) wrote :

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

Revision history for this message
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...)

Revision history for this message
Josh Stompro (u-launchpad-stompro-org) wrote :

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>

Revision history for this message
Dan Scott (denials) wrote :

A test would be a very good idea!

Revision history for this message
Josh Stompro (u-launchpad-stompro-org) wrote :

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)
Changed in evergreen:
importance: Undecided → Wishlist
assignee: nobody → Galen Charlton (gmc)
Revision history for this message
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  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.