Bug 7847 - OPAC search dies with plack
Summary: OPAC search dies with plack
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low normal (vote)
Assignee: Dobrica Pavlinusic
QA Contact: Ian Walls
Depends on:
Blocks: 7172 7844
  Show dependency treegraph
Reported: 2012-03-29 14:49 UTC by Dobrica Pavlinusic
Modified: 2013-12-05 20:01 UTC (History)
4 users (show)

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

Bug 7847 Undefined subroutine &C4::Search::GetHiddenItemnumbers (742 bytes, patch)
2012-03-29 23:50 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7847 Undefined subroutine &C4::Search::GetHiddenItemnumbers (1013 bytes, patch)
2012-03-31 01:23 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 7847 - OPAC search dies with plack (874 bytes, patch)
2012-04-04 21:46 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
[Signed off] Bug 7847 - OPAC search dies with plack (942 bytes, patch)
2012-04-05 09:22 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 7847: OPAC search dies with Plack (2.37 KB, patch)
2012-05-28 23:30 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 7847: OPAC search dies with Plack (2.39 KB, patch)
2012-06-01 17:21 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Dobrica Pavlinusic 2012-03-29 14:49:54 UTC
Undefined subroutine &C4::Search::GetHiddenItemnumbers called at /srv/koha_fer/opac/../C4/Search.pm line 1627.
 at /srv/koha_fer/opac/opac-search.pl line 780
Comment 1 Chris Cormack 2012-03-29 23:50:11 UTC Comment hidden (obsolete)
Comment 2 Dobrica Pavlinusic 2012-03-31 01:23:28 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2012-03-31 02:55:56 UTC
Did you mean in discussion or signed off?
Comment 4 Dobrica Pavlinusic 2012-03-31 14:40:24 UTC
I meant signed off, since Bug 6027 fix also doesn't seem right so this one is lesser evil.
Comment 5 Ian Walls 2012-03-31 20:59:21 UTC
It looks like there are several ways to clear up this issue:

1) this patch

2) remove the C4/Items.pm dependency (which is circular) on C4/Search that is introduced by bug 6027 (patch to fix this attached there)

3) add qw(GetHiddenItemnumbers) to the use C4::Items call on line 33 of C4/Search
Comment 6 Dobrica Pavlinusic 2012-04-02 13:43:05 UTC
Ian, which of those do you prefer?
Comment 7 Ian Walls 2012-04-02 16:57:16 UTC
I'm inclined to remove any circular dependencies, so option #2 would be my preferred solution.  Its unclear from bug 6027 why that dependency was introduced, as Paul noted, and I'm not sure why I didn't catch it in QA...  sorry, everyone.
Comment 8 Paul Poulain 2012-04-04 13:31:35 UTC
QA comment: one-line patch, passed QA
Comment 9 Paul Poulain 2012-04-04 13:34:13 UTC
well, in fact, I had a look at bug 6027, and found

that says "This change fixes Bug 7847 without other changes, but I'm at loss why."

does it mean I should apply both patches ? is attachment 8733 [details] [review] obsolete ?

marking "failed QA until I've an answer
Comment 10 Dobrica Pavlinusic 2012-04-04 14:10:48 UTC
Only one of patches should be applied.

Ian prefers one from bug 6027 and now that two people took a look at it, it seems more logical to me also.

I will put this patch to "In discussion" but I can't change status of bug 6027 to "Needs sign-off" for some reason.
Comment 11 Dobrica Pavlinusic 2012-04-04 21:46:27 UTC Comment hidden (obsolete)
Comment 12 Alex Arnaud 2012-04-05 09:21:39 UTC
No comment. It works on both koha-standard and koha-plack

Signing off
Comment 13 Alex Arnaud 2012-04-05 09:22:34 UTC Comment hidden (obsolete)
Comment 14 Paul Poulain 2012-04-06 16:14:25 UTC
QA comment: tiny patch, passed QA
Comment 15 Julian Maurice 2012-04-12 13:46:28 UTC
I reopen this bug since there is a problem removing 'use C4::Search' from C4::Items.

C4::Items::GetAnalyticsCount needs C4::Search for SimpleSearch and NZanalyze.
In bug 7162, the patch modify DelOrder to use DelItemCheck instead of DelItem, but it causes perl to die. This can be bypassed adding 'use C4::Search' in the .pl file that call DelOrder, but I don't think it is the way to do it.

Looking at C4::Search, it seems that C4::Items is only used for C4::Items::get_authorised_value_images, which is unrelated to items, so I think this sub should be moved (probably in C4::Koha, with others authorised_value related subs) so that C4::Search won't need C4::Items anymore.
Comment 16 Paul Poulain 2012-04-20 10:03:22 UTC
I'm a little bit lost with this bug and bug 6027. Is there something that still needs to be pushed ?
Comment 17 Ian Walls 2012-04-20 10:26:54 UTC
C4::Search is used by C4::Items in current master, so that much is settled.  The original problem, however, is probably still relevant, as GetHiddenItemnumbers is not explicitly scoped to C4::Items::
Comment 18 Jared Camins-Esakov 2012-05-28 23:30:13 UTC Comment hidden (obsolete)
Comment 19 Paul Poulain 2012-06-01 17:21:46 UTC
Created attachment 9887 [details] [review]
Bug 7847: OPAC search dies with Plack

Due to a dependency cycle between C4::Search and C4::Items, searches
in the OPAC die spectacularly under Plack. This counter-patch extends
dpavlin's solution and replaces use with require for C4::Search in
C4::Items and for C4::Items in C4::Search.

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 20 Paul Poulain 2012-06-01 17:22:16 UTC
Testing :
 * BEFORE the patch
  - opac & staff without Plack = work fine
  - opac with Plack = Undefined subroutine &C4::Search::GetHiddenItemnumbers called at /home/paul/koha.dev/koha-community/C4/Search.pm line 1634.
 at /home/paul/koha.dev/koha-community/opac/opac-search.pl line 806
  - staff with Plack = work fine

 * AFTER the patch
  - both opac & staff work fine with or without Plack

signed off
Comment 21 Ian Walls 2012-06-04 16:06:30 UTC
Bends (but does not completely break) circular dependency between C4::Items and C4::Search by moving the inclusion into the specific subroutines that require the module.  That, along with some scoping, are the primary changes here.

As far as I can tell, these two subroutines are all that require the other modules, so this is safe.  marking as Passed QA.
Comment 22 Paul Poulain 2012-06-05 13:47:15 UTC
This patch, related to Plack and OPAC should be backported to 3.8 I think, as Plack is announced as working for OPAC in 3.8
Comment 23 Chris Cormack 2012-06-05 15:14:42 UTC
Paul is correct, pushing to 3.8.x, will be in 3.8.2