Created attachment 176151 [details] [review] Bug 38832: Remove code related to outer_servers_loop and outer_sup_servers_loop This is clearly dead code as it's never set in the controller file.
Created attachment 176152 [details] [review] Bug 38832: Remove authority search from catalogue/search This one is less obvious, how is it possible to request an authority search using catalogue/search.pl? Is it dead code or is it actually used somewhere?
Comment on attachment 176151 [details] [review] Bug 38832: Remove code related to outer_servers_loop and outer_sup_servers_loop Review of attachment 176151 [details] [review]: ----------------------------------------------------------------- ::: catalogue/search.pl @@ -320,5 @@ > - $template->param(outer_servers_loop => $primary_servers_loop,); > - > - my $secondary_servers_loop; > - $template->param(outer_sup_servers_loop => $secondary_servers_loop,); > - Yeah this code looks like it was never actually implemented. Added in 2007 but the variables never populated. Then partially removed in 2009. May as well remove the rest now. Note that this same dead code also exists in opac/opac-search.pl, although it looks like it's already been removed from the OPAC templates.
Comment on attachment 176152 [details] [review] Bug 38832: Remove authority search from catalogue/search Review of attachment 176152 [details] [review]: ----------------------------------------------------------------- I think this is probably dead code. If you try to trigger this code, it just breaks Koha in many cases, since it's hard-coded to only work with Personal Name authorities: 1. Go to /cgi-bin/koha/catalogue/search.pl?q=e&server=authorityserver 2. Get this error: Can't call method "subfield" on an undefined value at /kohadevbox/koha/catalogue/search.pl line 710 Technically, you can get it to sort of work with the following: 1. Go to /cgi-bin/koha/catalogue/search.pl?q=Alvarez&server=authorityserver 2. On the right hand side of the screen, you'll have a link for "Alvarez Felix, Enrique," under a plain text heading of "authorityserver" 3. Clicking on the link just takes you to search.pl though since the link isn't properly formed Given the age of the code, it looks like something that probably never worked right in Koha 3+ -- In some ways, it's an interesting idea. It could be interesting to do an asynchronous search against the authority server and present it in a nice way, but we'd want to implement it in a very different way I think...
If you go to /cgi-bin/koha/catalogue/search.pl?q=Alvarez&server=authorityserver&server=biblioserver you see biblio results and authority results, but again... this only works because of a carefully crafted query. It would break most of the time, so we'd be better off taking it out now anyway.
There's a couple of QA tool failures. I imagine the tidiness one could probably be ignored I guess, but that $page issue should be fixed. (Actually, I think neither of the $page variables are actually used anywhere...) Thanks for taking a look at the search scripts. They can clearly benefit from a review... -- FAIL catalogue/search.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 222, now: 274) FAIL valid "my" variable $page masks earlier declaration in same scope OK koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/advsearch.tt OK koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
Noting that these patches make /cgi-bin/koha/catalogue/search.pl?q=Alvarez&server=authorityserver&server=biblioserver the same as cgi-bin/koha/catalogue/search.pl?q=Alvarez So confirming that they do work.
Created attachment 176167 [details] [review] Bug 38832: Remove code related to outer_servers_loop and outer_sup_servers_loop This is clearly dead code as it's never set in the controller file. Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 176168 [details] [review] Bug 38832: Remove authority search from catalogue/search This one is less obvious, how is it possible to request an authority search using catalogue/search.pl? Is it dead code or is it actually used somewhere? Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 176169 [details] [review] Bug 38832: (QA follow-up) Remove unused $page variables Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 176336 [details] [review] Bug 38832: Remove code related to outer_servers_loop and outer_sup_servers_loop This is clearly dead code as it's never set in the controller file. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 176337 [details] [review] Bug 38832: Remove authority search from catalogue/search This one is less obvious, how is it possible to request an authority search using catalogue/search.pl? Is it dead code or is it actually used somewhere? Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 176338 [details] [review] Bug 38832: (QA follow-up) Remove unused $page variables Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Pushed for 25.05! Well done everyone, thank you!