Bug 21101 - Allow to disable XSLT cache
Summary: Allow to disable XSLT cache
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-23 10:18 UTC by Fridolin Somers
Modified: 2018-09-16 20:56 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 21101: Rename engine var in C4::XSLT (1.63 KB, patch)
2018-07-23 12:09 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21101: Allow to disable XSLT cache (3.25 KB, patch)
2018-07-23 12:10 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2018-07-23 10:18:55 UTC
Koha::XSLT_Handler has a cache.
When Plack is used, the XSLT display have a cache that can last several minutes.
This is good for performance but bad for XSLT files edition.

I propose a syspref to enable/disable this cache, enabled by default.
Comment 1 Fridolin Somers 2018-07-23 12:09:26 UTC
Created attachment 77180 [details] [review]
Bug 21101: Rename engine var in C4::XSLT

'engine' is both a method, used outisde module, and a module shared variable.

I rename the variable 'xslth' to avoid confusion and use 'engine()' method inside the module.
Comment 2 Fridolin Somers 2018-07-23 12:10:26 UTC
Created attachment 77181 [details] [review]
Bug 21101: Allow to disable XSLT cache

Koha::XSLT_Handler has a cache.
When Plack is used, the XSLT display have a cache that can last several minutes.
This is good for performance but bad for XSLT files edition.

I propose a syspref to enable/disable this cache, enabled by default.

Test plan :
1) Enable Plack
2) Check syspref XSLTCache is enabled
3) Define an XSLT file path in some XSLT display, ie OPACXSLTResultsDisplay
4) Perform a search and look at results
5) Edit XSLT file to add some text, for example next to title
6) Perform a search and look at results => The change is not there, you can try several refresh
7) Undo action 5)
2) Set syspref XSLTCache disabled
8) Perform a search and look at results => you see original display
9) Repeat action 5)
10) Perform a search and look at results => you see the change, you can try several refreshs
Comment 3 Marcel de Rooy 2018-07-23 12:54:07 UTC
Do we really need a pref here? Why? Are these xslt files edited that regularly? If you edit a script, you need to restart Plack. If you edit a XSLT file, just do the same?
Comment 4 Owen Leonard 2018-07-23 12:57:26 UTC
(In reply to Marcel de Rooy from comment #3)
> Do we really need a pref here?

This is the justification I thought of after asking myself the same question: If the XSLT preferences are there to empower the librarian to make their own changes to XSLT, they need a way to test without having server access. Not a very common occurrence, but valid.
Comment 5 Paul Poulain 2018-07-23 16:14:09 UTC
Most of our customers have access to XSLT editing (through a webdave space), but they don't have any SSH access.
=> restarting plack & updating XSLT don't require the same kind of permission. With this dev, a librarian can :
 * turn the pref ON
 * play with XSLT and test
 * once it's OK, turn pref OFF and get caching & performance back.

The library is happy and our support team is happy too, one less thing to do :D
Comment 6 Tomás Cohen Arazi (tcohen) 2018-07-31 02:14:19 UTC
(In reply to Paul Poulain from comment #5)
> Most of our customers have access to XSLT editing (through a webdave space),
> but they don't have any SSH access.
> => restarting plack & updating XSLT don't require the same kind of
> permission. With this dev, a librarian can :
>  * turn the pref ON
>  * play with XSLT and test
>  * once it's OK, turn pref OFF and get caching & performance back.
> 
> The library is happy and our support team is happy too, one less thing to do
> :D

We need a couple 'flush' buttons in about.pl, what do you think?

- flush memcached
- flush template-toolkit cache
- flush XSLT cache
Comment 7 Marcel de Rooy 2018-07-31 12:19:15 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> We need a couple 'flush' buttons in about.pl, what do you think?
> 
> - flush memcached
> - flush template-toolkit cache
> - flush XSLT cache

Hmm. This might be better than toggling preferences.
Where: About or Administration?
The need to flush the XSLT cache separately might be very limited (although supported with $xslt->refresh). If we gracefully(!) restart Plack, we achieve the same and more?
Comment 8 Tomás Cohen Arazi (tcohen) 2018-08-02 13:35:10 UTC
Marcel: in the about page :-D
Comment 9 Fridolin Somers 2018-09-16 20:56:18 UTC
(In reply to Marcel de Rooy from comment #7)
> (In reply to Tomás Cohen Arazi from comment #6)
> > We need a couple 'flush' buttons in about.pl, what do you think?
> > 
> > - flush memcached
> > - flush template-toolkit cache
> > - flush XSLT cache
> 
> Hmm. This might be better than toggling preferences.
> Where: About or Administration?
> The need to flush the XSLT cache separately might be very limited (although
> supported with $xslt->refresh). If we gracefully(!) restart Plack, we
> achieve the same and more?

We need at least one for memcached and one for plack.
Note that starman has a "reload" option that can be implemented by sending to the starman master pid : /bin/kill -HUP <pid>

But I'm still in favor of a syspref because editing XSLT must be done in a lot of steps meaning :
 - Modify something
 - Save
 - Refreshing the HTML page

Avoid a step with cache flush is I think the best.