Summary: | Calling Koha::Article::Status::* without "use" in Patron.pm can cause breakage | ||
---|---|---|---|
Product: | Koha | Reporter: | Andrew Isherwood <bugzilla> |
Component: | Architecture, internals, and plumbing | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | 1joynelson, black23, hayleypelham, jonathan.druart, lucas, magnus, martin.renvoize, nick |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
20.05.00, 19.11.05, 19.05.10
|
|
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 23112 | ||
Attachments: |
Bug 23384: Fix "strict subs" error
Bug 23384: Fix "strict subs" error Bug 23384: Fix "strict subs" error [ALTERNATIVE-PATCH] Bug 23384: Fix use statement order for ArticleRequest::Status Bug 23384: Fix use statement order for ArticleRequest::Status |
Description
Andrew Isherwood
2019-07-26 13:31:45 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. Hi Andrew, can you provede test plan for this patch? Thank you. 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 (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. 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. 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> 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 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 I think this alternative patch is the way to go. Koha::ArticleRequests was missing from Koha::Patron 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" 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> Tested with bug 23112, all works as expected, small change, passing QA Nice work everyone! Pushed to master for 20.05 Backported to 19.11.x for 19.11.05 backported to 19.05.x for 19.05.10 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 |