Bug 38832 - Dead code in catalogue/search.pl
Summary: Dead code in catalogue/search.pl
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Pedro Amorim
URL:
Keywords:
Depends on:
Blocks: 38714
  Show dependency treegraph
 
Reported: 2025-01-06 15:03 UTC by Jonathan Druart
Modified: 2025-01-16 16:42 UTC (History)
3 users (show)

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


Attachments
Bug 38832: Remove code related to outer_servers_loop and outer_sup_servers_loop (4.47 KB, patch)
2025-01-06 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38832: Remove authority search from catalogue/search (6.94 KB, patch)
2025-01-06 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38832: Remove code related to outer_servers_loop and outer_sup_servers_loop (4.52 KB, patch)
2025-01-06 23:42 UTC, David Cook
Details | Diff | Splinter Review
Bug 38832: Remove authority search from catalogue/search (6.99 KB, patch)
2025-01-06 23:42 UTC, David Cook
Details | Diff | Splinter Review
Bug 38832: (QA follow-up) Remove unused $page variables (1.30 KB, patch)
2025-01-06 23:42 UTC, David Cook
Details | Diff | Splinter Review
Bug 38832: Remove code related to outer_servers_loop and outer_sup_servers_loop (4.58 KB, patch)
2025-01-10 14:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38832: Remove authority search from catalogue/search (7.05 KB, patch)
2025-01-10 14:25 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38832: (QA follow-up) Remove unused $page variables (1.36 KB, patch)
2025-01-10 14:25 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-01-06 15:03:57 UTC

    
Comment 1 Jonathan Druart 2025-01-06 15:06:39 UTC
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.
Comment 2 Jonathan Druart 2025-01-06 15:06:42 UTC
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 3 David Cook 2025-01-06 23:12:10 UTC
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 4 David Cook 2025-01-06 23:28:34 UTC
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...
Comment 5 David Cook 2025-01-06 23:31:01 UTC
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.
Comment 6 David Cook 2025-01-06 23:36:49 UTC
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
Comment 7 David Cook 2025-01-06 23:37:32 UTC
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.
Comment 8 David Cook 2025-01-06 23:42:35 UTC
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>
Comment 9 David Cook 2025-01-06 23:42:37 UTC
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>
Comment 10 David Cook 2025-01-06 23:42:40 UTC
Created attachment 176169 [details] [review]
Bug 38832: (QA follow-up) Remove unused $page variables

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 11 Pedro Amorim 2025-01-10 14:25:33 UTC
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>
Comment 12 Pedro Amorim 2025-01-10 14:25:35 UTC
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>
Comment 13 Pedro Amorim 2025-01-10 14:25:38 UTC
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>
Comment 14 Katrin Fischer 2025-01-16 16:42:21 UTC
Pushed for 25.05!

Well done everyone, thank you!