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".

Why replace https with http ?

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.

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.

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

(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.

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.

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 ?

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.

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

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 :)

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! :)

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?

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

It looks like a duplicate of bug 8952.

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