Bug 23384 - Calling Koha::Article::Status::* without "use" in Patron.pm can cause breakage
Summary: Calling Koha::Article::Status::* without "use" in Patron.pm can cause breakage
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 23112
  Show dependency treegraph
 
Reported: 2019-07-26 13:31 UTC by Andrew Isherwood
Modified: 2020-04-14 23:12 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10


Attachments
Bug 23384: Fix "strict subs" error (714 bytes, patch)
2019-07-26 13:35 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23384: Fix "strict subs" error (741 bytes, patch)
2019-10-09 10:42 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23384: Fix "strict subs" error (780 bytes, patch)
2020-02-14 15:50 UTC, Owen Leonard
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 23384: Fix use statement order for ArticleRequest::Status (1.59 KB, patch)
2020-02-18 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23384: Fix use statement order for ArticleRequest::Status (1.65 KB, patch)
2020-03-06 11:39 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2019-07-26 13:31:45 UTC
After spending a while digging into this, I can't explain why it's a problem, but I know that it is and I know how to fix it.

During the development of Bug 23112 I encountered a weird problem whereby adding "use Illrequests" to C4/Circulation.pm causes the whole of the app to break with the following error:

Bareword "Koha::ArticleRequest::Status::Pending" not allowed while "strict subs" in use at /home/koha-koha/kohaclone/Koha/Patron.pm line 811.
Bareword "Koha::ArticleRequest::Status::Processing" not allowed while "strict subs" in use at /home/koha-koha/kohaclone/Koha/Patron.pm line 811.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/Illrequest.pm line 37.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/Illrequest.pm line 37.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/Illrequests.pm line 23.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/Illrequests.pm line 23.
Compilation failed in require at /home/koha-koha/kohaclone/C4/Circulation.pm line 49.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/C4/Circulation.pm line 49.
Compilation failed in require at /home/koha-koha/kohaclone/C4/Overdues.pm line 31.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/C4/Overdues.pm line 31.
Compilation failed in require at /home/koha-koha/kohaclone/C4/Members.pm line 32.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/C4/Members.pm line 32.
Compilation failed in require at /home/koha-koha/kohaclone/C4/Letters.pm line 30.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/C4/Letters.pm line 30.
Compilation failed in require at /home/koha-koha/kohaclone/C4/Suggestions.pm line 29.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/C4/Suggestions.pm line 29.
Compilation failed in require at /home/koha-koha/kohaclone/C4/Acquisition.pm line 25.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/C4/Acquisition.pm line 25.
Compilation failed in require at /home/koha-koha/kohaclone/C4/Budgets.pm line 27.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/C4/Budgets.pm line 27.
Compilation failed in require at /home/koha-koha/kohaclone/C4/Biblio.pm line 94.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/C4/Biblio.pm line 94.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/Biblio.pm line 27.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/Biblio.pm line 27.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/Biblios.pm line 26.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/Biblios.pm line 26.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/ArticleRequest.pm line 26.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/ArticleRequest.pm line 26.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/ArticleRequests.pm line 26.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/ArticleRequests.pm line 26.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/Patrons.pm line 28.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/Patrons.pm line 28.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/Item.pm line 33.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/Item.pm line 33.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/Items.pm line 26.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/Items.pm line 26.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/Checkout.pm line 28.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/Checkout.pm line 28.
Compilation failed in require at /home/koha-koha/kohaclone/Koha/Checkouts.pm line 25.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/Koha/Checkouts.pm line 25.
Compilation failed in require at /home/koha-koha/kohaclone/C4/Auth.pm line 37.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/C4/Auth.pm line 37.
Compilation failed in require at /home/koha-koha/kohaclone/admin/preferences.pl line 23.
BEGIN failed--compilation aborted at /home/koha-koha/kohaclone/admin/preferences.pl line 23.

The reference to "preferences.pl" is a red herring and just reflects the page you're on at the time. The rest of the error is the same regardless of the page.

Poking around, I've not been able to determine why the usage of Illrequest in Circulation.pm would cause Patron.pm to suddenly complain about Koha::ArticleRequest::Status::* calls being barewords. With Modern::Perl the strict subs pragma should catch that sort of stuff. So I've really got no idea what's going on.

Anyway, the obvious fix is to add "use Koha::ArticleRequest::Status;" to Patron.pm, which is what the patch for this bug is going to do. I just wish I understood why it's a problem in the first place!
Comment 1 Andrew Isherwood 2019-07-26 13:35:21 UTC
Created attachment 91811 [details] [review]
Bug 23384: Fix "strict subs" error

This patch explicitly imports Koha::ArticleRequest::Status to avoid the
strict subs error described in the bug.
Comment 2 Michal Denar 2019-09-12 20:30:00 UTC
Hi Andrew,
can you provede test plan for this patch?

Thank you.
Comment 3 Andrew Isherwood 2019-09-13 08:59:55 UTC
Test plan:

- Edit "use Illrequests;" to the top of C4/Circulation.pm
=> TEST: Observe that no page in Koha will now load, and errors as described above
- Add "use Koha::ArticleRequest::Status;" to Patron.pm
=> TEST: Observe that the problem is now fixed.
- Scratch your head and wonder what the heck is going on
Comment 4 Magnus Enger 2019-10-03 08:29:50 UTC
(In reply to Andrew Isherwood from comment #3)
> - Edit "use Illrequests;" to the top of C4/Circulation.pm

You mean Koha::Illrequests, right? 

I still cant reproduce the problem. I did this:

$ git diff
diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 014c018628..ae3e1bbc0e 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -24,6 +24,7 @@ use strict;
 use DateTime;
 use POSIX qw( floor );
 use Koha::DateUtils;
+use Koha::Illrequests;
 use C4::Context;
 use C4::Stats;
 use C4::Reserves;

Did a restart_all in kohadevbox. But nothing obviously bad happens. I can still check out a book, for example.
Comment 5 Andrew Isherwood 2019-10-09 10:42:38 UTC
Created attachment 93912 [details] [review]
Bug 23384: Fix "strict subs" error

This patch explicitly imports Koha::ArticleRequest::Status to avoid the
strict subs error described in the bug.
Comment 6 Owen Leonard 2020-02-14 15:50:48 UTC
Created attachment 98965 [details] [review]
Bug 23384: Fix "strict subs" error

This patch explicitly imports Koha::ArticleRequest::Status to avoid the
strict subs error described in the bug.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Owen Leonard 2020-02-14 15:53:58 UTC
I signed this off because following this test plan is consistent with what the bug report says, with the assumption that the goal of the change is to allow at some future date the addition of "use Koha::Illrequests;" to Circulation.pm.

1. Add "use Koha::Illrequests;" to the top of C4/Circulation.pm
2. TEST: Nothing loads, the koha-plack-error log is stuck in an apparent loop
3. Add "use Koha::ArticleRequest::Status;" to Patron.pm
4. TEST: Pages are loading correctly again
Comment 8 Jonathan Druart 2020-02-18 15:16:25 UTC
Created attachment 99191 [details] [review]
[ALTERNATIVE-PATCH] Bug 23384: Fix use statement order for ArticleRequest::Status

Koha::ArticleRequest is used by Koha::ArticleRequests so
Koha::ArticleRequest::Status should not be needed in
Koha::ArticleRequest.
Also Koha::ArticleRequest::Status must be loaded before
Koha::ArticleRequest
Comment 9 Jonathan Druart 2020-02-18 15:17:08 UTC
I think this alternative patch is the way to go. Koha::ArticleRequests was missing from Koha::Patron
Comment 10 Andrew Isherwood 2020-02-19 08:31:54 UTC
Sounds like you did a better job of getting to the bottom of the problem than I did Jonathan! At least your patch has some reasoning behind it, whereas mine was just "I don't know why this works, but it does"
Comment 11 Nick Clemens 2020-03-06 11:39:27 UTC
Created attachment 100264 [details] [review]
Bug 23384: Fix use statement order for ArticleRequest::Status

Koha::ArticleRequest is used by Koha::ArticleRequests so
Koha::ArticleRequest::Status should not be needed in
Koha::ArticleRequest.
Also Koha::ArticleRequest::Status must be loaded before
Koha::ArticleRequest

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2020-03-06 11:40:17 UTC
Tested with bug 23112, all works as expected, small change, passing QA
Comment 13 Martin Renvoize 2020-03-06 15:02:35 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-04-02 16:33:24 UTC
Backported to 19.11.x for 19.11.05
Comment 15 Lucas Gass 2020-04-09 20:51:12 UTC
backported to 19.05.x for 19.05.10
Comment 16 Hayley Mapley 2020-04-14 23:12:39 UTC
Unable to replicate this issue on 18.11.x, not backporting for now - if you can replicate on 18.11.x let me know and I will apply it