Having to write [% KOHA_VERSION %] for each url is bad because: - It's easily forgettable when adding new <script> or <link> - It prevents grep'ing for the full filename - It violates the DRY principle - If at some point we want to change the "force js and css reload" mechanism, it will be tedious
Created attachment 73791 [details] [review] Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere Having to write [% KOHA_VERSION %] for each url is bad because: - It's easily forgettable when adding new <script> or <link> - It prevents grep'ing for the full filename - It violates the DRY principle - If at some point we want to change the "force js and css reload" mechanism, it will be tedious This patch: - adds a Template::Toolkit plugin that generates <script> and <link> tags for JS and CSS files, and inserts automatically the Koha version in the filename - use the new plugin to remove all occurences of [% KOHA_VERSION %] - remove the code that was adding KOHA_VERSION as a template variable Test plan: 1. Apply patch 2. Go to several different pages in Koha (opac and intranet) while checking your browser's dev tools (there should be no 404 for JS and CSS files, and the Koha version should appear in filenames) and the server logs (there should be no "File not found") 3. `git grep KOHA_VERSION` should return nothing 4. prove t/db_dependent/Koha/Template/Plugin/Asset.t
Patch doesn't apply for me on master, sorry
Created attachment 73836 [details] [review] Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere Having to write [% KOHA_VERSION %] for each url is bad because: - It's easily forgettable when adding new <script> or <link> - It prevents grep'ing for the full filename - It violates the DRY principle - If at some point we want to change the "force js and css reload" mechanism, it will be tedious This patch: - adds a Template::Toolkit plugin that generates <script> and <link> tags for JS and CSS files, and inserts automatically the Koha version in the filename - use the new plugin to remove all occurences of [% KOHA_VERSION %] - remove the code that was adding KOHA_VERSION as a template variable Test plan: 1. Apply patch 2. Go to several different pages in Koha (opac and intranet) while checking your browser's dev tools (there should be no 404 for JS and CSS files, and the Koha version should appear in filenames) and the server logs (there should be no "File not found") 3. `git grep KOHA_VERSION` should return nothing 4. prove t/db_dependent/Koha/Template/Plugin/Asset.t
Patch rebased on master
Just in case, the sed commands I used to modify (almost) all templates: sed -i -E 's/<(script|link) .*(href|src)=["'"'"']((\[%.*?%\])\/)*([^%]*)_\[% KOHA_VERSION %\]\.(js|css)["'"'"'].*(\/?>|<\/\1>)/[% Asset.\6("\5\.\6") %]/' **/*.inc **/*.tt git status --porcelain | grep '^ M' | awk '{print $2}' | xargs -- sed -i '1i[% USE Asset %]'
Created attachment 74048 [details] [review] Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere Having to write [% KOHA_VERSION %] for each url is bad because: - It's easily forgettable when adding new <script> or <link> - It prevents grep'ing for the full filename - It violates the DRY principle - If at some point we want to change the "force js and css reload" mechanism, it will be tedious This patch: - adds a Template::Toolkit plugin that generates <script> and <link> tags for JS and CSS files, and inserts automatically the Koha version in the filename - use the new plugin to remove all occurences of [% KOHA_VERSION %] - remove the code that was adding KOHA_VERSION as a template variable Test plan: 1. Apply patch 2. Go to several different pages in Koha (opac and intranet) while checking your browser's dev tools (there should be no 404 for JS and CSS files, and the Koha version should appear in filenames) and the server logs (there should be no "File not found") 3. `git grep KOHA_VERSION` should return nothing 4. prove t/db_dependent/Koha/Template/Plugin/Asset.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 74049 [details] [review] Bug 20538: (follow-up) Update discharges request page with Asset plugin This is follow-up of bug 20524 Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
I started to write a rule for coding guidelines here: https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML8:_use_Asset_TT_plugin_for_linking_javascript_and_css_files feel free to update it and we can discuss it on the next meeting
There are some FAILs in the QA script, but I assume they are just side effects/false positives for this patch. Maybe something to look into later: FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-MARCdetail.tt FAIL forbidden patterns forbidden pattern: opac-tmpl should certainly replaced with [% interface %] (line 67) OK git manipulation OK spelling OK tt_valid OK valid_template FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-detail.tt FAIL forbidden patterns forbidden pattern: opac-tmpl should certainly replaced with [% interface %] (line 132) OK git manipulation OK spelling OK tt_valid OK valid_template ... 1) Is the copyright statement correct? +package Koha::Template::Plugin::Asset; + +# Copyright Marc Véron / marc veron ag, Switzerland 2) Not blocker, but a note: Need to update qa tools! Can you quickly check 1) please?
Created attachment 74053 [details] [review] Bug 20538: Fix copyright notice in Asset.pm
Created attachment 74054 [details] [review] Bug 20538: Fix 2 JS paths
(In reply to Katrin Fischer from comment #9) > There are some FAILs in the QA script, but I assume they are just side > effects/false positives for this patch. Maybe something to look into later: > > FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-MARCdetail.tt > FAIL forbidden patterns > forbidden pattern: opac-tmpl should certainly replaced with [% interface > %] (line 67) > OK git manipulation > OK spelling > OK tt_valid > OK valid_template > > FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth-detail.tt > FAIL forbidden patterns > forbidden pattern: opac-tmpl should certainly replaced with [% interface > %] (line 132) > OK git manipulation > OK spelling > OK tt_valid > OK valid_template > ... These ones are true positives. Fixed in the last patch > > 1) Is the copyright statement correct? > +package Koha::Template::Plugin::Asset; > + > +# Copyright Marc Véron / marc veron ag, Switzerland > > 2) Not blocker, but a note: Need to update qa tools! > > Can you quickly check 1) please? Done. Thanks for catching that.
Created attachment 74056 [details] [review] Bug 20538: Move categories.js out of language directory This was the last js/css file inside a language directory
Created attachment 74057 [details] [review] Bug 20538: Add POD for Asset.pm
I wrote a little patch for qa-test-tools to avoid all the 'File not found' warnings. You can found it here : https://github.com/jajm/koha-qa-test-tools/tree/bug_20538
Created attachment 74058 [details] [review] Bug 20538: Remove the need of writing [% KOHA_VERSION %] everywhere Having to write [% KOHA_VERSION %] for each url is bad because: - It's easily forgettable when adding new <script> or <link> - It prevents grep'ing for the full filename - It violates the DRY principle - If at some point we want to change the "force js and css reload" mechanism, it will be tedious This patch: - adds a Template::Toolkit plugin that generates <script> and <link> tags for JS and CSS files, and inserts automatically the Koha version in the filename - use the new plugin to remove all occurences of [% KOHA_VERSION %] - remove the code that was adding KOHA_VERSION as a template variable Test plan: 1. Apply patch 2. Go to several different pages in Koha (opac and intranet) while checking your browser's dev tools (there should be no 404 for JS and CSS files, and the Koha version should appear in filenames) and the server logs (there should be no "File not found") 3. `git grep KOHA_VERSION` should return nothing 4. prove t/db_dependent/Koha/Template/Plugin/Asset.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74059 [details] [review] Bug 20538: (follow-up) Update discharges request page with Asset plugin This is follow-up of bug 20524 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74060 [details] [review] Bug 20538: Fix copyright notice in Asset.pm Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74061 [details] [review] Bug 20538: Fix 2 JS paths Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74062 [details] [review] Bug 20538: Move categories.js out of language directory This was the last js/css file inside a language directory Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74063 [details] [review] Bug 20538: Add POD for Asset.pm Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 74171 [details] [review] Bug 20538: ->tag is private - tell it explicitly
Pushed to master for 18.05, thanks to everybody involved!
Could you take a look at xt/author/valid-templates.t ? There are too many errors: File not found : lib/emoji-picker/js/emoji-picker.js at /home/vagrant/kohaclone/Koha/Template/Plugin/Asset.pm line 83. File not found : js/basket.js at /home/vagrant/kohaclone/Koha/Template/Plugin/Asset.pm line 83. File not found : js/tags.js at /home/vagrant/kohaclone/Koha/Template/Plugin/Asset.pm line 83. File not found : js/datatables.js at /home/vagrant/kohaclone/Koha/Template/Plugin/Asset.pm line 83. [...]
Created attachment 74835 [details] [review] Bug 20538: Prevent warnings in xt/author/valid-templates.t
(In reply to Julian Maurice from comment #25) > Created attachment 74835 [details] [review] [review] > Bug 20538: Prevent warnings in xt/author/valid-templates.t Pushed to master, thanks Julian!
Enhancement, not backported for 17.11
*** Bug 20066 has been marked as a duplicate of this bug. ***