Add timestamp to CSS and JS files for when they are built

[jmacgreg]

... to force browser caches to clear. From Nate:

We could do the same with a timestamp based on when the file was built: ../style.css?t=0193938788.... Alternatively, we could bake the timestamp into the filename itself since we compile directly: ../style-019384.css. I'm tempted to go the latter route, because I believe a URL with a query string prevents some versions of server-side caching of requests from working properly (the query string has to be evaluated). however, that might not matter to us since we're using a PHP script to load the CSS file anyway

[NateWr]

Yeah so just to follow up on my previous comment, since we use a URL like ...$$$call$$$/page/page/css, and it hits a PHP file, and then we load and return the file contents, it may not matter that much what the filename is. It might be possible just to add a query string onto it based on the time the file was last compiled: $$$call$$$/page/page/css?t=012924894589.

It would be nice to have a URL that points to a static file directly, but that kind of minor performance bump doesn't seem like a big priority for us at the moment.

[NateWr]

A quick diff which appends the version of the app into the query string:

diff --git a/classes/template/PKPTemplateManager.inc.php b/classes/template/PKPTemplateManager.inc.php
index df01185..5240391 100644
--- a/classes/template/PKPTemplateManager.inc.php
+++ b/classes/template/PKPTemplateManager.inc.php
@@ -1433,6 +1433,9 @@ class PKPTemplateManager extends SmartyBC {
                        $context = 'frontend';
                }

+               $versionDao = DAORegistry::getDAO('VersionDAO');
+               $appVersion = $versionDao->getCurrentVersion()->getVersionString();
+
                $stylesheets = $this->getResourcesByContext($this->_styleSheets, $params['context']);

                ksort($stylesheets);
@@ -1443,6 +1446,9 @@ class PKPTemplateManager extends SmartyBC {
                                if (!empty($style['inline'])) {
                                        $output .= '<style type="text/css">' . $style['style'] . '</style>';
                                } else {
+                                       if (strpos($style['style'], '?') === false) {
+                                               $style['style'] .= '?v=' . $appVersion;
+                                       }
                                        $output .= '<link rel="stylesheet" href="' . $style['style'] . '" type="text/css" />';
                                }
                        }
@@ -1465,6 +1471,9 @@ class PKPTemplateManager extends SmartyBC {
                        $params['context'] = 'frontend';
                }

+               $versionDao = DAORegistry::getDAO('VersionDAO');
+               $appVersion = $versionDao->getCurrentVersion()->getVersionString();
+
                $scripts = $this->getResourcesByContext($this->_javaScripts, $params['context']);

                ksort($scripts);
@@ -1475,6 +1484,9 @@ class PKPTemplateManager extends SmartyBC {
                                if ($data['inline']) {
                                        $output .= '<script type="text/javascript">' . $data['script'] . '</script>';
                                } else {
+                                       if (strpos($data['script'], '?') === false) {
+                                               $data['script'] .= '?v=' . $appVersion;
+                                       }
                                        $output .= '<script src="' . $data['script'] . '" type="text/javascript"></script>';
                                }
                        }

With this approach we end up with URLs to static assets like /1.11.0/jquery.min.js?v=3.1.1 which are confusing and unnecessary. Might be worth thinking about whether we should pass a version with the asset registration, so this can be controlled better. Or whether that's all just too much premature optimistaion.

[ajnyga]

Hi @NateWr

After upgrading to 3.1.1.2 I was getting so many questions about lost manuscripts (the submissions listings were missing because of old scripts loading from browser cache) I had to take the code you have above to production use.

Do you see any potential problems? Because I can see you were not too happy about the solution.

[NateWr]

@ajnyga I think you'll be fine running that in production. My nitpicks are more about how third-parties might want to interact with their asset URLs over the long-term but it shouldn't matter for a one-off deployment.

[ajnyga]

Just pointing out that this issue comes up on the forum fairly often, for example https://forum.pkp.sfu.ca/t/after-upgrading-from-2-4-8-1-to-ojs-3-1-1-4-loading-problem/48948/4.

We have been using the quick fix from Nate for 2 months now and works great. Hopefully it will be included in 3.2.

[asmecher]

@NateWr, I'm all in favour of merging that tweak and back-porting to ojs-stable-3_1_1 and omp-stable-3_1_1 if you agree.

[NateWr]

:+1: I agree. I have not been able to locate any firm information about caching issues and that's a pretty minimal side effect so let's just do it.

[ajnyga]

Can this go to 3.1.2?

[NateWr]

Yes, if you can provide a PR for it for master and stable-3_1_2.

[ajnyga]

Here is the pr for master branch: https://github.com/pkp/pkp-lib/pull/4439

Again, if I try to do a submodule update and a pull request in OJS, there are 15 commits visible there. Although I fetched latest master branch pushed to my own repository and then created a new branch for the pull request using the upstream/master branch. There is something I do not understand.

[ajnyga]

and then it just worked...

Master pkp-lib: https://github.com/pkp/pkp-lib/pull/4440 ojs submodule update: https://github.com/pkp/ojs/pull/2270

[ajnyga]

Stable-3_1_2 (possibly not so stable after this pr): pkp-lib: https://github.com/pkp/pkp-lib/pull/4441 ojs submodule update: https://github.com/pkp/ojs/pull/2271

[NateWr]

Otherwise it looks good @ajnyga. Happy to merge once the tests are passing. :+1:

[ajnyga]

if try to restart the tests it now says that there are conflicts, but does not let me to resolve them. I do not think I have the time to continue with this right now, I have a ton of work with our own service. So I guess just leave the 3.2 milestone there.

[NateWr]

Master: https://github.com/pkp/pkp-lib/pull/4449 https://github.com/pkp/ojs/pull/2273 (tests only)

3.1.2: https://github.com/pkp/pkp-lib/pull/4450 https://github.com/pkp/ojs/pull/2274 (tests only)

[asmecher]

@ajnyga, I think Nate's working to get the tests passing per the above comment, but about testing in general: it seems to have gotten less stable recently, leading to intermittent failures, plus we've recently added testing for disable_path_info mode which is not especially reliable. I'm working on this now (and will continue in the next couple of weeks) but if this is a blocker, please don't spend too much time on it -- bug me instead and I'll tackle it.

[ajnyga]

i think the tests above died in the same stage as yesterday although they should have the required check https://github.com/pkp/pkp-lib/pull/4449/files#diff-a52d1f134c8e119d3cdb2e280b60ac82R1509. I am busy with other journal.fi stuff for the next couple of days and the release date for 3.1.2 is coming rather quickly, so it is fine for me that this is left for next release. It is a easy patch to do locally when I upgrade to 3.1.2

[NateWr]

Not sure what's happening with the tests. I can't reproduce it and I'm unable to run the tests locally at the moment, so this probably won't make it in for 3.1.2.

[ajnyga]

Yes I noticed the same thing, can not see any problem when I try the changes locally. Maybe a problem in the tests itself? But as I said above, this is low priority. I can try a pr against the master branch at some point

[asmecher]

@NateWr and @ajnyga, I'd be happy to walk through some test debugging with either or both of you -- if that would help, hit me up on Slack and we can schedule something shortly.

[mfelczak]

Hi @asmecher and @NateWr, any chance we could schedule this against the upcoming 3.2 release (it's currently tagged against 3.3)?

[asmecher]

@mfelczak, I suspect so.

@NateWr, do you want to walk through the testing stuff sometime? A rebase by itself might resolve it -- the testing stuff has evolved quite a bit since February.

[NateWr]

PRs (master): https://github.com/pkp/pkp-lib/pull/4831 https://github.com/pkp/ojs/pull/2422 (tests only)

[NateWr]

Merged and cherry-picked to stable.

[edyrhaug]

Hi guys! I've been talking with @jnugent and he asked me to comment on this issue.

Background

I made a similar change to this back in 2018, where the appended query string had to be manually incremented. This let us force clients to refresh css and js assets whenever we wanted, which was typically when we had made changes to our theme, but we also used it for the case discussed here where the app itself had been updated with breaking changes.

This PR is a very welcome one, in that it automatically handles the app update case, but unfortunately it does not cover the case where the theme is updated.

When I first was made aware of this PR I initially merged these changes with my own so that we would get automatic cache busting when the app version had changed but we would still be able to manually increment a string to bust the cache on theme changes. But I since decided I should try to make my part automatic as well, so that we could stop touching this file in the core pkp library.

Intent and current state

What I wanted to do was to fetch the name and version of the currently active theme and append that to the app version string that this PR adds. I was able to do this, but then I discovered that updating the version number in the theme's version.xml does nothing towards updating the versions table in the database, so I'm stuck at whatever the theme plugin version was when it was first added.

So now I'm wondering whether there is a way to systematically refresh the version info for the active theme plugin in the database, or if this has to be handled in some other way. I could perhaps circumvent the versions table entirely by fetching the version info from version.xml and assign it to a variable in our ThemePlugin.inc.php? (something, something, $templateMgr->assign('themeVersion', $themeVersion);) But then this would only work for our theme, meaning these changes to PKPTemplateManager would have to remain local, and we would have to keep track of them. :(

Here follows my local changes as they currently stand (this is live on our sandbox site btw @NateWr @asmecher).

Effect of current changes:

• Append query string to css or js calls even when they already contain a query string, by using &v= instead of ?v=
• Don't append a query string to third party assets (fonts, jquery and popper)
• Append name and version of currently active theme (resulting in a combined query string that looks like ?v=3.1.2.0_default1.0.0.0)

Issues:

• Updating the plugin's version.xml does not update this output

Diff:

--- PKPTemplateManager.inc.php.stable-3_1_2
+++ PKPTemplateManager.inc.php.local
@@ -1435,8 +1435,16 @@
            $params['context'] = 'frontend';
        }

+       //Get current theme and app version:
+       $themes = PluginRegistry::loadCategory('themes', true);
+       foreach($themes as $theme) {
+           if ($theme->isActive()) {
+               $cv = $theme->getCurrentVersion()->{'_data'};
+               $themeVersion = $cv['product'] . $cv['major'] . '.' . $cv['minor'] . '.' . $cv['revision'] . '.' . $cv['build'];
+           }
+       }
        $versionDao = DAORegistry::getDAO('VersionDAO');
-       $appVersion = defined('SESSION_DISABLE_INIT') ? null : $versionDao->getCurrentVersion()->getVersionString();
+       $appVersion = defined('SESSION_DISABLE_INIT') ? null : $versionDao->getCurrentVersion()->getVersionString() . '_' . $themeVersion;

        $stylesheets = $this->getResourcesByContext($this->_styleSheets, $params['context']);

@@ -1448,8 +1456,12 @@
                if (!empty($style['inline'])) {
                    $output .= '<style type="text/css">' . $style['style'] . '</style>';
                } else {
-                   if (strpos($style['style'], '?') === false) {
-                       $style['style'] .= '?v=' . $appVersion;
+                   if  (strpos($style['style'], 'font') == false) {
+                       if (strpos($style['style'], '?') !== false) {
+                           $style['style'] .= '&v=' . $appVersion;
+                       } else {
+                           $style['style'] .= '?v=' . $appVersion;
+                       }
                    }
                    $output .= '<link rel="stylesheet" href="' . $style['style'] . '" type="text/css" />';
                }
@@ -1515,8 +1527,16 @@
            $params['context'] = 'frontend';
        }

+       //Get current theme and app version:
+       $themes = PluginRegistry::loadCategory('themes', true);
+       foreach($themes as $theme) {
+           if ($theme->isActive()) {
+               $cv = $theme->getCurrentVersion()->{'_data'};
+               $themeVersion = $cv['product'] . $cv['major'] . '.' . $cv['minor'] . '.' . $cv['revision'] . '.' . $cv['build'];
+           }
+       }
        $versionDao = DAORegistry::getDAO('VersionDAO');
-       $appVersion = defined('SESSION_DISABLE_INIT') ? null : $versionDao->getCurrentVersion()->getVersionString();
+       $appVersion = defined('SESSION_DISABLE_INIT') ? null : $versionDao->getCurrentVersion()->getVersionString() . '_' . $themeVersion;

        $scripts = $this->getResourcesByContext($this->_javaScripts, $params['context']);

@@ -1528,8 +1548,12 @@
                if ($data['inline']) {
                    $output .= '<script type="text/javascript">' . $data['script'] . '</script>';
                } else {
-                   if (strpos($data['script'], '?') === false) {
-                       $data['script'] .= '?v=' . $appVersion;
+                   if  (strpos($data['script'], 'jquery') == false || strpos($data['script'], 'popper') == false) {
+                       if (strpos($data['script'], '?') !== false) {
+                           $data['script'] .= '&v=' . $appVersion;
+                       } else {
+                           $data['script'] .= '?v=' . $appVersion;
+                       }
                    }
                    $output .= '<script src="' . $data['script'] . '" type="text/javascript"></script>';
                }

[NateWr]

Thank you @edyrhaug for your post. This is a tricky issue because I don't think we want to load and process every plugin's version.xml file on every request. But that is a problem if the version's diverge and the database is out of sync.

I've filed a separate issue for this which you can track here: https://github.com/pkp/pkp-lib/issues/4841

[mfelczak]

@edyrhaug, would it be possible for you to simply run lib/pkp/tools/installPluginVersion.php after you have updated your theme to update the DB versions table?

[edyrhaug]

I sure can @mfelczak! That seems like a good solution to me. Afaict the script adds a new row in the versions table and sets this as the "current" for the plugin in question. This is probably what would happen if I updated the plugin by uploading a new version in the web interface too?

What do you guys think - can we go ahead with this then? It seems my proposed changes will work nicely after all? Theme developers just have to know about the installPluginVersion script if they want to update locally or through git, while it would work automatically for people who update the theme they are using by uploading a new version through the web ui?

[NateWr]

I've moved the conversation over to https://github.com/pkp/pkp-lib/issues/4841#issuecomment-505775922 since this issue is closed.

[asmecher]

These changes broke the installer for me (on stable-3_1_2, newest ojs and pkp-lib repos). It looks like the fetch of VersionDAO attempts to create a database connection, leading to...

Fatal error: Uncaught Error: Call to undefined function mysql_connect() in /home/asmecher/git/ojs-stable-3_1_2/lib/pkp/lib/adodb/drivers/adodb-mysql.inc.php:456 Stack trace: #0 /home/asmecher/git/ojs-stable-3_1_2/lib/pkp/lib/adodb/adodb.inc.php(558): ADODB_mysql->_connect('localhost', 'ojs', 'ojs', 'ojs') #1 /home/asmecher/git/ojs-stable-3_1_2/lib/pkp/classes/db/DBConnection.inc.php(151): ADOConnection->Connect('localhost', 'ojs', 'ojs', 'ojs', false) #2 /home/asmecher/git/ojs-stable-3_1_2/lib/pkp/classes/db/DBConnection.inc.php(126): DBConnection->connect() #3 /home/asmecher/git/ojs-stable-3_1_2/lib/pkp/classes/db/DBConnection.inc.php(83): DBConnection->initConn() #4 /home/asmecher/git/ojs-stable-3_1_2/lib/pkp/classes/db/DBConnection.inc.php(52): DBConnection->initDefaultDBConnection() #5 /home/asmecher/git/ojs-stable-3_1_2/lib/pkp/classes/db/DBConnection.inc.php(227): DBConnection->__construct() #6 /home/asmecher/git/ojs-stable-3_1_2/lib/pkp/classes/db/DBConnection.inc.php(238): DBConnection::getInstance() #7 /home/asme in /home/asmecher/git/ojs-stable-3_1_2/lib/pkp/lib/adodb/drivers/adodb-mysql.inc.php on line 456

This should fix it: https://github.com/pkp/pkp-lib/pull/4910

[NateWr]

Oops! I'm curious why it wasn't caught by the tests: don't the integration tests go through installation?

[asmecher]

Yes, the tests do run through installation, so it should've been caught -- but maybe nobody has committed a submodule update since this was merged. Looking at https://github.com/pkp/ojs/pull/2422 the tests were never run for some reason.