Bug 7307 - OPACBaseURL not used in opac-search.pl
Summary: OPACBaseURL not used in opac-search.pl
Status: RESOLVED DUPLICATE of bug 8952
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.6
Hardware: All All
: P3 normal (vote)
Assignee: Adrien SAURAT
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-02 10:17 UTC by Adrien SAURAT
Modified: 2015-02-20 16:21 UTC (History)
6 users (show)

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


Attachments
proposed patch (1.40 KB, patch)
2011-12-02 13:16 UTC, Adrien SAURAT
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7307: use of the OPACBaseURL syspref for RSS feed (1.72 KB, patch)
2011-12-04 18:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
proposed patch #2 (8.25 KB, patch)
2012-01-06 09:59 UTC, Adrien SAURAT
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Adrien SAURAT 2011-12-02 10:17:17 UTC
Some users could have a problem with the RSS feed if they have a specific base url, as the system preference OPACBaseURL is not used yet in opac-search.pl.
Comment 1 Adrien SAURAT 2011-12-02 12:57:45 UTC
To reproduce the bug :

1) Admin : type a value in the OPACBaseURL system preference.

2) Opac : do a search on anything, then click on the RSS button. The url will stay the same, when it should be modified by the system preference.


The OPACBaseURL is already implemented in the template file "opac-opensearch.tt". The only thing lacking is the value transfer in "opac-search.pl".
Comment 2 Adrien SAURAT 2011-12-02 13:16:14 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2011-12-04 18:34:02 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2011-12-05 09:05:20 UTC
Why replace https with http ?
Comment 5 Adrien SAURAT 2011-12-05 14:01:06 UTC
I don't know and the developper of the first version of this patch (developped for a specific customer) can't remember why he did this. I guess we can remove this, I'll make another patch.

The following check :
if ( $OPACRssURL !~ m/^http:\/\// ) { $OPACRssURL = "http://" . $OPACRssURL }
will then have to handle https cases.
Comment 6 Adrien SAURAT 2011-12-05 14:04:57 UTC
In fact, the syspref text says the following :

The OPAC is located at http://[INPUT ZONE] . (This must be filled in correctly for RSS, unAPI, and search plugins to work.) 

The http part is not supposed to be typed, and only the addition of "http://" seems to be expected.
Comment 7 Paul Poulain 2011-12-14 17:17:05 UTC
QA comment : 
Adrien,

I think you've found a problem with this syspref !

But if I push your patch like this, then anyone with an https opac won't have rss enabled.

So, please resubmit a patch with this code removed.
you can also replace the 
+    if ( $OPACRssURL !~ m/^http:\/\// ) { $OPACRssURL = "http://" . $OPACRssURL }

by something like
+    if ( $OPACRssURL !~ m/^http(s?):\/\// ) { $OPACRssURL = "http://" . $OPACRssURL }

I think (untested regexp!)

marking failed QA, when new patch submitted, switch back to signed-off, the change is small
Comment 8 Chris Cormack 2011-12-14 17:20:37 UTC
(In reply to comment #7)
> QA comment : 
> Adrien,
> 
> I think you've found a problem with this syspref !
> 
> But if I push your patch like this, then anyone with an https opac won't have
> rss enabled.
> 
> So, please resubmit a patch with this code removed.
> you can also replace the 
> +    if ( $OPACRssURL !~ m/^http:\/\// ) { $OPACRssURL = "http://" .
> $OPACRssURL }
> 
> by something like
> +    if ( $OPACRssURL !~ m/^http(s?):\/\// ) { $OPACRssURL = "http://" .
> $OPACRssURL }
> 
> I think (untested regexp!)
> 
Hmm what if the site is not served on http:// that will then be a 404 error?

It's a tricky one that is for sure.
Comment 9 Adrien SAURAT 2011-12-20 08:50:16 UTC
Maybe it would be best to leave the whole path to be configured by the user, including the http or https part ?

This would imply changing the syspref hint and making sure that other implementations of OPACBaseURL would still work.
Comment 10 Adrien SAURAT 2012-01-06 09:32:29 UTC
I'm preparing a patch where the whole path has to be stored in the system preference, but I have a doubt about C4/Auth.pm, which contains the following code at line 445 :

  OPACBaseURL               => ($in->{'query'}->https() ? "https://" : "http://") . $ENV{'SERVER_NAME'} .
                   ($ENV{'SERVER_PORT'} eq ($in->{'query'}->https() ? "443" : "80") ? '' : ":$ENV{'SERVER_PORT'}"),


Should I replace this by a simple :

  OPACBaseURL => C4::Context->preference('OPACBaseURL')

or is there a reason to keep such a construction with SERVER_PORT ?
Comment 11 Adrien SAURAT 2012-01-06 09:36:12 UTC
Well, in fact OPACBaseURL is not necessarily filled with a value, so I guess I'll just use this code when it's empty, otherwise we use what's stored in the system preference.
Comment 12 Adrien SAURAT 2012-01-06 09:59:29 UTC
Created attachment 7062 [details] [review]
proposed patch #2

Effects of the patch :
1) OPACBaseURL must now contain the full address with http:// or https://
(system preference help changed accordingly)
2) The RSS Feed in OPAC results now handle OPACBaseURL
Comment 13 Katrin Fischer 2012-01-06 11:09:04 UTC
Hm, not sure here, but will this change break the URL for installations where no full URL is entered? 
Can we make the update take care about it? (preferred)
Or have a note in the update process that people have to change the preference?
Should staffClientBaseURL be changed to work like that as well?
Sorry for so many questions :)
Comment 14 Adrien SAURAT 2012-01-06 11:36:46 UTC
1) Hm, not sure here, but will this change break the URL for installations where
no full URL is entered? 
-> Yes.

2) Can we make the update take care about it? (preferred)
-> Not totally safe, we could assume that we can add "http://" at the beginning of the existing value, this would solve most cases. But the point of this "full URL" change is that some people may need "https" instead, and we have no way to decide between http and https.

3) Or have a note in the update process that people have to change the preference?
-> Would be good, but where could this appear ? I don't know very well this part yet (I'm creating a new field for another patch so I'm discovering the update process, but what if we don't create a field, as here ?).

4) Should staffClientBaseURL be changed to work like that as well?
-> Didn't know this one !
A grep on the whole koha code shows only one actual use of this preference, in "/misc/load_testing/benchmark_circulation.pl", in the following line:
my $baseurl= C4::Context->preference("staffClientBaseURL")."/cgi-bin/koha/"
It's already working like this new version of OPACBaseURL, and the "staffClientBaseURL" hint only says "Specify the base URL of the staff client", so I guess we're fine on this one.

5) Sorry for so many questions :)
Kein Problem! :)
Comment 15 Ian Walls 2012-03-02 16:36:05 UTC
I'm with Katrin on this; we need to be sure this doesn't break existing installations that don't have the http:// or https:// in the system preference.  Until that is resolved, this needs to be marked Failed QA.

Some additional problems:

Broken TT tag around line 38 of koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/sendshelf.tt :   % OPACBaseURL %]

OPACrssURL is needlessly defined and assigned; we're just passing OPACBaseURL straight through to the template param of the same name.  Why have an extra storage variable for this?
Comment 16 David Cook 2012-06-12 06:50:03 UTC
Since Koha already has http hard-coded in, why not just replace it with another system preference where Koha admins can specify whether it's http or https? 

I found it already hard-coded in: 

virtualshelves/sendshelf.tt
/xslt/MARC21slim2intranetDetail.xsl
/modules/opac-sendshelf.tt

I had to add it to the following to get the RSS feeds (both the link and the actual feed, since it relied on opac-search.pl for OPACBaseURL) to work in the OPAC (after initializing OPACBaseURL in opac-search.pl):

/includes/masthead.inc
Comment 17 Jonathan Druart 2015-02-20 16:21:54 UTC
It looks like a duplicate of bug 8952.

*** This bug has been marked as a duplicate of bug 8952 ***