Bug 4032 - XSLT systempreference should take a path to file rather than YesNo
Summary: XSLT systempreference should take a path to file rather than YesNo
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.8
Hardware: All All
: P3 enhancement (vote)
Assignee: Stéphane Delaune
QA Contact: Marcel de Rooy
URL:
Keywords:
: 4447 (view as bug list)
Depends on: 6919
Blocks: 4447 7821
  Show dependency treegraph
 
Reported: 2010-01-07 08:42 UTC by Henri-Damien LAURENT
Modified: 2013-12-05 20:01 UTC (History)
15 users (show)

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


Attachments
patch proposed (10.60 KB, patch)
2010-01-07 09:14 UTC, Chris Cormack
Details | Diff | Splinter Review
proposed patch (14.70 KB, patch)
2012-02-03 16:06 UTC, Stéphane Delaune
Details | Diff | Splinter Review
proposed patch if 6919 is apply before 4032 (1.18 KB, patch)
2012-02-03 16:07 UTC, Stéphane Delaune
Details | Diff | Splinter Review
proposed patch (16.82 KB, patch)
2012-02-08 11:20 UTC, Stéphane Delaune
Details | Diff | Splinter Review
proposed patch (16.20 KB, patch)
2012-02-27 13:49 UTC, Stéphane Delaune
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 4032:XSLT systempreference takes a path to file rather than YesNo (16.36 KB, patch)
2012-03-21 14:46 UTC, Marijana Glavica
Details | Diff | Splinter Review
Bug 4032 XSLT systempreference takes a path to file rather than YesNo (16.38 KB, patch)
2012-03-24 17:46 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:22:21 UTC


---- Reported by henridamien@koha-fr.org 2010-01-07 08:42:25 ----

managing XSLT filename should be a systempreference 
It would enable ppl to change XSLT.



---- Additional Comments From henridamien@koha-fr.org 2010-01-07 09:14:56 ----

Created an attachment
patch proposed





---- Additional Comments From henridamien@koha-fr.org 2010-01-07 09:16:01 ----

This Bug also affects 3.2 But since it also takes Intranet stylesheets, patch is different.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:22 UTC  ---

This bug was previously known as _bug_ 4032 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4032
Imported an attachment (id=1791)

Actual time not defined. Setting to 0.0
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.
The original submitter of attachment 1791 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Galen Charlton 2010-05-25 16:05:35 UTC
Marking as enhancement and pushing to 3.4.
Comment 2 Julian Maurice 2011-03-18 13:21:48 UTC
Patch does not apply.
--
Applying: (bug #3042) Changing XSLT sysprefs to take filename
/home/julian/koha/versions/koha-community-git/.git/rebase-apply/patch:68: trailing whitespace.
    $string = get $uri ; 
error: patch failed: C4/Search.pm:1338
error: C4/Search.pm: patch does not apply
error: patch failed: C4/XSLT.pm:26
error: C4/XSLT.pm: patch does not apply
error: patch failed: installer/data/mysql/en/mandatory/sysprefs.sql:204
error: installer/data/mysql/en/mandatory/sysprefs.sql: patch does not apply
error: patch failed: installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql:206
error: installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql: patch does not apply
error: installer/data/mysql/updatedatabase30.pl: does not exist in index
error: patch failed: kohaversion.pl:10
error: kohaversion.pl: patch does not apply
error: patch failed: opac/opac-detail.pl:70
error: opac/opac-detail.pl: patch does not apply
Patch failed at 0001 (bug #3042) Changing XSLT sysprefs to take filename
Comment 3 Julian Maurice 2011-03-18 13:24:02 UTC
A little precision: (In reply to comment #2)
> Patch does not apply.
> --
> Applying: (bug #3042) Changing XSLT sysprefs to take filename
> [...]
> Patch failed at 0001 (bug #3042) Changing XSLT sysprefs to take filename

A little precision: the bug number is wrong but this is the good patch.
Comment 4 dev_patches 2011-03-18 16:02:19 UTC
The patch was based on 3.0
So could not work
Comment 5 Chris Cormack 2011-07-07 21:51:45 UTC
If this is still a problem needs patches based on master
Comment 6 Stéphane Delaune 2012-02-03 16:06:37 UTC Comment hidden (obsolete)
Comment 7 Stéphane Delaune 2012-02-03 16:07:44 UTC Comment hidden (obsolete)
Comment 8 Stéphane Delaune 2012-02-03 16:12:57 UTC
i made a new patch , if the 6919 is apply (before mine), you'll need to apply the "proposed patch if 6919 is apply before 4032 " to make compatibility between them
Comment 9 Stéphane Delaune 2012-02-06 10:08:23 UTC
*** Bug 4447 has been marked as a duplicate of this bug. ***
Comment 10 Liz Rea 2012-02-06 15:21:17 UTC
I feel it is unclear what needs to be done with this preference - it may be a documentation thing.

What do you put in if you want to use the default xslt stylesheet? If you don't want to use XSLT at all?

Would it be better to have a hybrid approach - a drop down to select XSLT and a box to put in the path to your custom stylesheet?
Comment 11 Stéphane Delaune 2012-02-06 15:52:47 UTC
(In reply to comment #10)
> I feel it is unclear what needs to be done with this preference - it may be a
> documentation thing.
> 
> What do you put in if you want to use the default xslt stylesheet?
if syspref was true, the updatedatabase insert default xslt stylesheet as new value

>If you don't want to use XSLT at all?

for not use it, leave a blank value (no value) in the syspref

> 
> Would it be better to have a hybrid approach - a drop down to select XSLT and a
> box to put in the path to your custom stylesheet?
why use 8 sysprefs while only 4 are sufficient.

no value/empty value = no xslt ; a value = the path to xslt file to use

I do not find it complicated to understand, I do not think it is appropriate to double the number of sysprefs here
Comment 12 Stéphane Delaune 2012-02-06 16:00:17 UTC
(In reply to comment #10)

> What do you put in if you want to use the default xslt stylesheet?
e.g. if you are in unimarc, with koha src into /home/koha/src for the syspref XSLTDetailsDisplay :
/home/koha/src/koha-tmpl/intranet-tmpl/prog/en/xslt/UNIMARCslim2intranetDetail.xsl

as I said in my previous comment : if syspref was true (value=1), the updatedatabase insert default xslt stylesheet as new value (path built with the same method previously used by C4::XSLT->XSLTParse4Display )
Comment 13 Liz Rea 2012-02-06 16:33:27 UTC
I think this will confuse your average user, and the software is for them, not us. 

This patch would need two things to get my signoff:

1. better documentation in the syspref description 
2. no 1/0 on/off usage. This is so confusing for your average user, and considering that most libraries will use the default xslt (*not* define a specific stylesheet), unnecessarily so.
Comment 14 Stéphane Delaune 2012-02-07 07:59:32 UTC
the interest of the xslt is especially to be able to customize the display, and thus to define a different xslt than the default, and thus to define a specific stylesheet. Am I wrong ?
Comment 15 Paul Poulain 2012-02-07 08:22:09 UTC
(In reply to comment #13)
> I think this will confuse your average user, and the software is for them, not
> us.  
> This patch would need two things to get my signoff:
> 1. better documentation in the syspref description 
> 2. no 1/0 on/off usage. This is so confusing for your average user, and
> considering that most libraries will use the default xslt (*not* define a
> specific stylesheet), unnecessarily so.

We had a discussion about your comments with Stéphane.
* I don't see clearly the difference between display without and the default xslt. I'm wondering if you're not implying to make the default XSLT being the default display. In this case, an easy change would be to have those sysprefs being for specific XSLT, if they're empty, then use default XSLT
* Stéphane is right about XSLT made for specific display. Many/most of our libraries want some tweaks on XSLT. So we use 2 options only: no XSLT or specific XSLT
* there is also a problem with XSLT syspref as we've today : on a server with more than 1 Koha (using koha-tools), it's impossible to have a specific XSLT for each setup

A simple answer to your proposition would be to change the behaviour and have 3 options:
* empty => no XLST
* the string "default" or "1" => default XSLT
* a full XSL path => the specific XSL you've defined.
it could easily be documented on syspref.pl page
Would you be OK with this ?

Note that what is great with this patch is also that we can have full URLs (http://xslt.koha-community.org/nekls.xsl for example), so we could share XSLT as we're already sharing SQL queries on the server. Add a meaningfull wiki page, pointing all existing XSLT and their look, and you get a nice feature
Comment 16 Katrin Fischer 2012-02-07 09:11:01 UTC
I think the problem is, when people are not using xslt views now but want to turn it on later. It will mean they can't just switch it on but have to put in a complicated string - and if they do it wrong, what will happen?
Comment 17 Katrin Fischer 2012-02-07 09:12:37 UTC
I missed your last comments - I think 'default' could work, when it's documented in the syspref description.
Comment 18 Liz Rea 2012-02-07 15:49:11 UTC
I think we may have had something lost in translation :)

I am *absolutely in favor* of allowing custom XSLT stylesheets. That's great functionality.

I was thinking of a set of sysprefs that combine the current functionality 

"Display results <normally || using xslt>" with the "add a path for custom xslt." 

The end result would look like:

"Display results <normally || using xslt>, using this custom XSLT stylesheet <entry box>. Leave box empty to use the default xslt stylesheet"

This way all current functionality is retained, and I don't think it's too confusing.
Comment 19 Paul Poulain 2012-02-07 17:51:02 UTC
(In reply to comment #18)
> "Display results <normally || using xslt>, using this custom XSLT stylesheet
> <entry box>. Leave box empty to use the default xslt stylesheet"
afaik (but I may be wrong), this can't be done with only 1 syspref, we need 2. And one of them is useless intellectually.
Would the following description be OK ?

Display results using <entry box> XSLT. Leave empty for "no xslt", enter "default" for the default one, put an URL for an external specific stylesheet.
Comment 20 Liz Rea 2012-02-07 17:59:50 UTC
You are right, you would require 2 sysprefs.

However, there is precedent for doing it in the way I describe: http://screencast.com/t/LMrkLMIxl

If you insist upon keeping it to a single preference, this:

Display results using <entry box> XSLT. Leave empty for "no xslt", enter
"default" for the default one, put an URL for an external specific stylesheet.

Would be fine with me.
Comment 21 Stéphane Delaune 2012-02-08 11:20:45 UTC Comment hidden (obsolete)
Comment 22 Stéphane Delaune 2012-02-08 11:38:22 UTC
bad copy/paste in my last comment, sorry ; the correct text for the options is :

The four sysprefs have now for options:
- leave empty for "no xslt"
- enter "default" for the default one
- put a path to define a xslt file
- put an URL for an external specific stylesheet.
Comment 23 Liz Rea 2012-02-08 17:53:30 UTC
I could not get this patch to apply: 


Applying: Bug 4032:XSLT systempreference takes a path to file rather than YesNo
fatal: sha1 information is lacking or useless (C4/Search.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 4032:XSLT systempreference takes a path to file rather than YesNo
Comment 24 Stéphane Delaune 2012-02-09 10:11:11 UTC
(In reply to comment #23)
> I could not get this patch to apply: 
> 
> 
> Applying: Bug 4032:XSLT systempreference takes a path to file rather than YesNo
> fatal: sha1 information is lacking or useless (C4/Search.pm).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Bug 4032:XSLT systempreference takes a path to file rather
> than YesNo

this patch depend on 6919. You need to apply 6919 befor apply this ; have try this ?
Comment 25 Stéphane Delaune 2012-02-27 13:49:16 UTC Comment hidden (obsolete)
Comment 26 Owen Leonard 2012-03-09 16:37:50 UTC
I tried putting the OPAC xsl files on a different server and linking to them via a full URL and I'm getting this error:

I/O warning : failed to load external entity "MARC21slimUtils.xsl"
compilation error: file unknown-a19e318 element import
xsl:import : unable to load MARC21slimUtils.xsl
 at /home/oleonard/kohaclone/C4/XSLT.pm line 214

I have copied MARC21slimUtils.xsl to the same remote directory as the OPAC detail xsl file. Is there something else I'm missing?
Comment 27 Stéphane Delaune 2012-03-13 13:41:34 UTC
(In reply to comment #26)
> I tried putting the OPAC xsl files on a different server and linking to them
> via a full URL and I'm getting this error:
> 
> I/O warning : failed to load external entity "MARC21slimUtils.xsl"
> compilation error: file unknown-a19e318 element import
> xsl:import : unable to load MARC21slimUtils.xsl
>  at /home/oleonard/kohaclone/C4/XSLT.pm line 214
> 
> I have copied MARC21slimUtils.xsl to the same remote directory as the OPAC
> detail xsl file. Is there something else I'm missing?

It is normal, it come from your xsl file that uses a relative path.
if your syspref OPACXSLTDetailsDisplay have as value : "http://www.myurl.com/myxslfiles/MARC21slim2OPACDetail.xsl"
you need edit this file (MARC21slim2OPACDetail.xsl) to replace the line :
<xsl:import href="MARC21slimUtils.xsl"/>
with this :
<xsl:import href="http://www.myurl.com/myxslfiles/MARC21slimUtils.xsl"/>
Comment 28 Marijana Glavica 2012-03-21 10:29:10 UTC
I tested the patch. It is a very nice feature and I would like to have it very much. It works with remote URL and with local path.

But, there is a problem if I want to use more than one language on my site and that is usual situation in non-english speaking countries.
Currently I can have just one xslt file. What we need is different xslt files for every language. I don't see how this problem could be solved with remote URL, but in a case of local files, we could have possibility to write just the name of the local file, put them in xslt directory, and expect Koha to take care about proper path according to language settings. Similar situation already exists with local CSS files for OPAC and staff client.
Comment 29 Marijana Glavica 2012-03-21 14:46:43 UTC Comment hidden (obsolete)
Comment 30 Ian Walls 2012-03-22 13:42:35 UTC
So, it's looking like the trade-off is between multi-language support and remote URLs.  It would be easy enough to set the syspref to look at a theme and language specific directory, and provide a dropdown of available options.  All one would need to do is copy their XSLT there, and select it.  Easier than typing it in.

The cost of this would be remote URLs, since we couldn't manage them in the file structure like that.  The XSLT would need to be downloaded and saved in the right theme/language directory... not a terrible burden, but it would be incumbent on the sysadmin to keep the local copy synced with the remote copy.

While I only work in a single language situation, I think that multi-language support is more important than remote URLs.  Could others please supply their opinions?
Comment 31 Marijana Glavica 2012-03-22 14:41:00 UTC
For me, multi-language support is very important and I would not use this feature without it. Theoretically, I could use it for intranet, but I don't like the idea of having original files in my local language, I prefer current situation - all original files are in English and local language files are generated with translation tool.
Comment 32 Paul Poulain 2012-03-22 14:46:11 UTC
(In reply to comment #31)
> For me, multi-language support is very important and I would not use this
> feature without it. Theoretically, I could use it for intranet, but I don't
> like the idea of having original files in my local language, I prefer
> current situation - all original files are in English and local language
> files are generated with translation tool.

Well, the multilingual option is still working as without this patch : just use the "default" option.
No / default option => previous behaviour, exactly
url => new behaviour

I don't see why this patch should be discarded
Comment 33 Marijana Glavica 2012-03-22 14:53:53 UTC
(In reply to comment #32)
> Well, the multilingual option is still working as without this patch : just
> use the "default" option.
> No / default option => previous behaviour, exactly
> url => new behaviour
> 
> I don't see why this patch should be discarded

well.. that is why I signed-off the patch :)
still, I don't like it :)
Comment 34 Dobrica Pavlinusic 2012-03-23 08:33:50 UTC
Would including %s in remote URLs as placeholder for current language code be acceptable solution for multi-language without changing too much code?
Comment 35 Katrin Fischer 2012-03-23 08:46:00 UTC
Hi Dobrica, 
I would suggest using {langcode} or similar, to not confuse the translation tool and because we use that kind of placeholder in other systempreferences like OpacSearchForTitleIn.
Comment 36 Dobrica Pavlinusic 2012-03-24 17:20:57 UTC
Bug 7821 implements proposed {langcode}. We still have almost whole day to get this pushed in 3.8 :-)
Comment 37 Dobrica Pavlinusic 2012-03-24 17:46:26 UTC
Created attachment 8622 [details] [review]
Bug 4032 XSLT systempreference takes a path to file rather than YesNo

Signed-off-by: Marijana Glavica <mglavica@ffzg.hr>

I am signing it off because it doesn't break anything and I will report
another bug for language issues described in my previous comment.

Removed MySQLism backquotes
Comment 38 Paul Poulain 2012-03-29 10:15:08 UTC
QA comment = medium size patch, nothing specific to spot, coded correctly

proposing passed QA, but, as it's a BibLibre patch, not setting the status
Comment 39 Marcel de Rooy 2012-03-29 11:16:39 UTC
Like this patch. Just have the impression that we once had this already? Anyhow, looks good. Two minor remarks:

Error checking: What happens when GetURI returns undef on a URL? Could you fall back to default or no xslt view? 
Now you have a perl error: Empty String at /usr/share/koha/testclone/C4/XSLT.pm line 208
This needs a followup.

You test Biblibre by: SELECT 1 FROM systempreferences WHERE variable="IntranetXSLTDetailsDisplay"
Just theoretically, but someone else using this local pref could run into problems.. No blocker.

Passed QA
Comment 40 Paul Poulain 2012-03-29 15:04:41 UTC
patch pushed. The "biblibre test" stuff is related to how we called at first the syspref, and it have been made live for some of our customers. We would be really unlucky if somone had had the idea of developing something with a local syspref that has the same name !!! (or, we're very lucky and should go to Las Vegas ;-) )
Comment 41 Nicole C. Engard 2012-03-30 13:03:43 UTC
Documentation question - is this field looking for a full URL? http://xxx.com or a path to a folder on Koha? /xxx/x.xsl ?
Comment 42 Paul Poulain 2012-03-30 13:11:40 UTC
(In reply to comment #41)
> Documentation question - is this field looking for a full URL?
> http://xxx.com or a path to a folder on Koha? /xxx/x.xsl ?

Both ! You can write
/path/to/koha/and/your/stylesheet.xsl
as well as
http://koha-community.org/shared/xsl/publiclibrary.xsl