Also, if you have a reverse proxy in front of Apache and you proxy using HTTP rather than HTTPS, Apache will generate fully qualified HTTP URLs for the Redirect, which cause one of the two scenarios:

Scenario 1:
1) Client (C) requests HTTPS from Reverse Proxy (RP)
2) RP requests HTTP from Apache (A)
3) A sends HTTP redirect to RP
4) RP sends HTTP redirect to C
5) C requests HTTP from RP

Now you've just downgraded your HTTPS to HTTP by accident

Scenario 2 (Force HTTPS):
1) Client (C) requests HTTPS from Reverse Proxy (RP)
2) RP requests HTTP from Apache (A)
3) A sends HTTP redirect to RP
4) RP sends HTTP redirect to C
5) C requests HTTP to RP
6) RP sends HTTPS redirect to C

Now you've just redirected twice bouncing from HTTPS to HTTP to HTTPS for just 1 search.

Created attachment 105136 [details] [review]
Bug 25548: Remove Apache rewrite directives that trigger redirects

Historically, we have used Apache rewrite directives to strip
query parameters with no arguments from the query
string (e.g. PARAM=&), and then redirect to the new URL.

However, this slows down user activity, and depending on your
HTTPS and proxy setup can cause suboptimal HTTP downgrades and
upgrades.

Test Plan:
0) Apply patch
1) Build Debian package
2) Install Debian package and reload Apache
3) Open developer tools on your favourite browser
4) Go to Network tab in developer tools
5) Go to /cgi-bin/koha/opac-main.pl
6) Search for "test"
7) Note a 200 status for "opac-search.pl?idx=&q=test"

If you got a 302 status for "opac-search.pl?idx=&q=test"
followed by a 200 status for "opac-search.pl?q=test", you
will know that your Apache configuration wasn't updated.

Ah and it causes Bug 7607

+1 to remove useless rewrites.

Frido, maybe a sign-off then? :)

Created attachment 110984 [details] [review]
Bug 25548: (follow-up) Remove those rewrite redirects in koha-httpd.conf

In etc/koha-httpd.conf for dev install, those rewrite redirects are already commented.
Just remove them.

Created attachment 110985 [details] [review]
Bug 25548: Remove Apache rewrite directives that trigger redirects

Historically, we have used Apache rewrite directives to strip
query parameters with no arguments from the query
string (e.g. PARAM=&), and then redirect to the new URL.

However, this slows down user activity, and depending on your
HTTPS and proxy setup can cause suboptimal HTTP downgrades and
upgrades.

Test Plan:
0) Apply patch
1) Build Debian package
2) Install Debian package and reload Apache
3) Open developer tools on your favourite browser
4) Go to Network tab in developer tools
5) Go to /cgi-bin/koha/opac-main.pl
6) Search for "test"
7) Note a 200 status for "opac-search.pl?idx=&q=test"

If you got a 302 status for "opac-search.pl?idx=&q=test"
followed by a 200 status for "opac-search.pl?q=test", you
will know that your Apache configuration wasn't updated.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Created attachment 110986 [details] [review]
Bug 25548: (follow-up) Remove those rewrite redirects in koha-httpd.conf

In etc/koha-httpd.conf for dev install, those rewrite redirects are already commented.
Just remove them.

(In reply to Katrin Fischer from comment #4)
> Frido, maybe a sign-off then? :)

Allez ;)

Tested on Strech with Koha 20.05

Looks like this rewite is disabled in koha-httpd.conf since Koha 3.12 ^^

(In reply to Fridolin SOMERS from comment #9)
> Looks like this rewite is disabled in koha-httpd.conf since Koha 3.12 ^^

I think that you and I were the ones who did that one back in the day! Heh.

And thanks for the sign off and follow-up!

Created attachment 111062 [details] [review]
Bug 25548: Remove Apache rewrite directives that trigger redirects

Historically, we have used Apache rewrite directives to strip
query parameters with no arguments from the query
string (e.g. PARAM=&), and then redirect to the new URL.

However, this slows down user activity, and depending on your
HTTPS and proxy setup can cause suboptimal HTTP downgrades and
upgrades.

Test Plan:
0) Apply patch
1) Build Debian package
2) Install Debian package and reload Apache
3) Open developer tools on your favourite browser
4) Go to Network tab in developer tools
5) Go to /cgi-bin/koha/opac-main.pl
6) Search for "test"
7) Note a 200 status for "opac-search.pl?idx=&q=test"

If you got a 302 status for "opac-search.pl?idx=&q=test"
followed by a 200 status for "opac-search.pl?q=test", you
will know that your Apache configuration wasn't updated.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 111063 [details] [review]
Bug 25548: (follow-up) Remove those rewrite redirects in koha-httpd.conf

In etc/koha-httpd.conf for dev install, those rewrite redirects are already commented.
Just remove them.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Looks good to me too. But a rather intriguing question is: is there some script somewhere that depended somehow on the logic of removed empty parameters?
(Apart from seeing the other way around on the see also bug.)
Have the impression that David already had this in production for longer, so it is probably highly fictitious ;)

(In reply to Marcel de Rooy from comment #13)
> Looks good to me too. But a rather intriguing question is: is there some
> script somewhere that depended somehow on the logic of removed empty
> parameters?
> (Apart from seeing the other way around on the see also bug.)

I've traced the Apache lines back to 2007 when MJ Ray added them via commit e449c139fff, but there's really no information provided. 

I imagine that it was just cosmetic. That empty URL query parameters look ugly so that they stripped them off. MJ or Chris Cormack (who signed off on the patch) might know more.

> Have the impression that David already had this in production for longer, so
> it is probably highly fictitious ;)

Yep, I've been running this in production since May 20th 2020.

Hum, funny how this one and bug 7607 end up PQA at the same time.
I won't push both as this one will fix the problem from bug 7607.

(In reply to David Cook from comment #0)
> It means that every simple search to /cgi-bin/koha/opac-search.pl from the
> masthead will redirect from /cgi-bin/koha/opac-search.pl?idx=&q=blah to
> /cgi-bin/koha/opac-search.pl?q=blah. 
> 
> It's OK if we have idx= in the URL. 

However this is not only having ONE idx=, an adv search will become

/cgi-bin/koha/catalogue/search.pl?advsearch=1&idx=kw&q=ti&idx=kw&q=&idx=kw&q=&limit=&limit=&limit=&limit=&limit=&sort_by=relevance

That's starting to be ugly.

was: 
/cgi-bin/koha/catalogue/search.pl?advsearch=1&idx=kw&q=ti&idx=kw&idx=kw&sort_by=relevance

This has been an historical behaviour, how can we know it won't impact people?

At the minimum we should comment them I'd say.

Tomas, Nick, what are your thoughts on this one?

(In reply to Jonathan Druart from comment #15)
> /cgi-bin/koha/catalogue/search.
> pl?advsearch=1&idx=kw&q=ti&idx=kw&q=&idx=kw&q=&limit=&limit=&limit=&limit=&li
> mit=&sort_by=relevance
> 
> That's starting to be ugly.

I agree that it is ugly. But does it really matter the user?

(In reply to Marcel de Rooy from comment #18)
> (In reply to Jonathan Druart from comment #15)
> > /cgi-bin/koha/catalogue/search.
> > pl?advsearch=1&idx=kw&q=ti&idx=kw&q=&idx=kw&q=&limit=&limit=&limit=&limit=&li
> > mit=&sort_by=relevance
> > 
> > That's starting to be ugly.
> 
> I agree that it is ugly. But does it really matter the user?

We show the lines in search history and other places... and users are sharing them on social media, in emails etc. I also think they are much less readable/guessable for users. I'd much prefer the shorter links.

(In reply to Katrin Fischer from comment #19)
> (In reply to Marcel de Rooy from comment #18)
> > (In reply to Jonathan Druart from comment #15)
> > > /cgi-bin/koha/catalogue/search.
> > > pl?advsearch=1&idx=kw&q=ti&idx=kw&q=&idx=kw&q=&limit=&limit=&limit=&limit=&li
> > > mit=&sort_by=relevance
> > > 
> > > That's starting to be ugly.
> > 
> > I agree that it is ugly. But does it really matter the user?
> 
> We show the lines in search history and other places... and users are
> sharing them on social media, in emails etc. I also think they are much less
> readable/guessable for users. I'd much prefer the shorter links.

Would it be reasonable to clear empty inputs with javascript before the submission?

(In reply to Nick Clemens from comment #20)
> Would it be reasonable to clear empty inputs with javascript before the
> submission?

I like your thinking here, Nick. It would be better to prevent creating ugly query strings rather than trying to fix them after the fact.

Plus, in a future where we may just use Starman/Hypnotoad to serve Koha, we won't be able on Apache URL rewriting trickery. 


So maybe Javascript to clear empty inputs with Javascript, or Javascript to build the request. The former would probably be easier to bolt on to the status quo.

Who is volunteering?

(In reply to Jonathan Druart from comment #22)
> Who is volunteering?

Hm, just clearning the inputs won't fix the problem of bug 7607 I think? The problem is, that people leave search fields empty so idx and search terms don't match up. 

kw - no search term entered
title - some title words

Turns into: kw search for some title words

We have found this workaround from the other bug to be working, but I am not sure if that will fix the ugliness of the URL.

$(document).ready(function(){
$("input[name='do']").click(function(){
    $("input[name='q']").each(function(i){
        if (!$(this).val()){
            $("select[name='idx']").eq(i).append('<option value="" selected="selected"></option>'); 
        }
    }); 
});
});

and in intranetuserjs :

$(document).ready(function() {
$("#submit1,#submit2").click(function(){
    $("input[name='q']").each(function(i){
        if (!$(this).val()){
            $("select[name='idx']").eq(i).append('<option value="" selected="selected"></option>'); 
        }
    }); 
});
});

(In reply to Nick Clemens from comment #20)
> Would it be reasonable to clear empty inputs with javascript before the
> submission?

On reflection, that won't work the way you think it would work. It would create the problems described in Bug 7607.

(In reply to Katrin Fischer from comment #23)
> (In reply to Jonathan Druart from comment #22)
> > Who is volunteering?
> 
> Hm, just clearning the inputs won't fix the problem of bug 7607 I think? The
> problem is, that people leave search fields empty so idx and search terms
> don't match up. 
> 
> kw - no search term entered
> title - some title words
> 
> Turns into: kw search for some title words
> 
> We have found this workaround from the other bug to be working, but I am not
> sure if that will fix the ugliness of the URL.

With the Apache change, you don't need to do the workaround. The URL is equally as ugly as well. 

At the moment, it looks like the only way of having a less ugly URL is by removing empty search boxes from the DOM.

I am tempted to move this back to "Passed QA" because it actually resolves the problem from Bug 7607. 

While it makes for uglier URLs, it is functionally superior.

(In reply to David Cook from comment #24)
> (In reply to Nick Clemens from comment #20)
> > Would it be reasonable to clear empty inputs with javascript before the
> > submission?
> 
> On reflection, that won't work the way you think it would work. It would
> create the problems described in Bug 7607.

It won't if you remove the related idx.

I don't think we should push this patch without having both sides happy.

(In reply to Jonathan Druart from comment #27)
> (In reply to David Cook from comment #24)
> > (In reply to Nick Clemens from comment #20)
> > > Would it be reasonable to clear empty inputs with javascript before the
> > > submission?
> > 
> > On reflection, that won't work the way you think it would work. It would
> > create the problems described in Bug 7607.
> 
> It won't if you remove the related idx.
> 

What I meant was that if you set the idx value to '' (ie if you "clear empty inputs"), it will actually create an inconsistency in the form submission where the idx param is *not* sent in the form submission but the q param *is* sent. That is, unless you include an option in the select list for the idx that has a value of ''. It seems like glitchy/unexpected browser behaviour (at least in Chrome). 

But, as you say, removing the elements from the HTML form all together would work.

> I don't think we should push this patch without having both sides happy.

Sounds all right to me. I already applied these patches locally about 6 months ago, so I don't mind too much. Happy to help with Bug 7607 and then we can push this one once that's sorted.

(In reply to Jonathan Druart from comment #27)
>
> I don't think we should push this patch without having both sides happy.

David Nind and I have signed off on Nick's patch on Bug 7607. Once QA pass it, then we should all be happy ^_^.

Moving back to Passed QA and adding a dependency on Bug 7607. Just so that it doesn't get lost.

Pushed to master for 20.11, thanks to everybody involved!

Pushed to 20.05.x for 20.05.07

Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.

(In reply to Victor Grousset/tuxayo from comment #33)
> Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.

It is needed!

This did actually break sql reports with empty parameters.

(In reply to Tomás Cohen Arazi from comment #35)
> This did actually break sql reports with empty parameters.

Yes, we just found out this was the reason why filtered reports didn't work as expected when leaving blank inputs... For example, If you define a report like this:

select <<Library|branches:all>> as lib, 
           <<Item type|itemtypes:all>> as itype,
           <<CC|CC:all>> as cc,
           <<Date from>> as date_from,
           <<Date to>> as date_to,
           <<Invoice>> as invoice

and submit the form with one parameter.. for example the one that says 'Invoice' the form posts to the report with the following query string parameters:

reports: 41
phase: Run this report
param_name: Library|branches:all
sql_params: %
param_name: Item type|itemtypes:all
sql_params: %
param_name: CC|CC:all
sql_params: %
param_name: Date from
sql_params: 
param_name: Date to
sql_params: 
param_name: Invoice
sql_params: invoice_1 


But after redirection, empty parameters are filtered, leaving things like this

reports: 41
phase: Run this report
param_name: Library|branches:all
sql_params: %
param_name: Item type|itemtypes:all
sql_params: %
param_name: CC|CC:all
sql_params: %
param_name: Date from
param_name: Date to
param_name: Invoice
sql_params: invoice_1

Notice missing sql_params. This ended up the query to be built like this:

select '%' as lib, 
           '%' as itype,
           '%' as cc,
           'invoice_1' as date_from,
           NULL as date_to,
           NULL as invoice

notice that 'invoice_1' is placed in date_from instead of invoice column.

This fix is more awesome than you realized, ;)

(In reply to Agustín Moyano from comment #36)
> This fix is more awesome than you realized, ;)

That's a good way to start the day!

Great news then :)

Backported to 19.11.x branch for 19.11.13