Summary: | ->pickup_locations should always be called in scalar context | ||
---|---|---|---|
Product: | Koha | Reporter: | Tomás Cohen Arazi <tomascohen> |
Component: | Architecture, internals, and plumbing | Assignee: | Tomás Cohen Arazi <tomascohen> |
Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | normal | ||
Priority: | P5 - low | CC: | andrewfh, jonathan.druart, katrin.fischer, kyle, martin.renvoize, tomascohen, victor |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
The Koha::Biblio->pickup_locations and Koha::Item->pickup_location methods don't always honour list context. Because of this, when used, they should assume scalar context. If list context was required, the developer needs to explicitly chain a ->as_list call.
This patch tracks the uses of this methods and adjusts accordingly.
|
Version(s) released in: |
22.05.00,21.11.03,21.05.11
|
Bug Depends on: | 29804, 29807 | ||
Bug Blocks: | 29844 | ||
Attachments: |
Bug 29806: Call ->as_list for ->pickup_locations on request.pl
Bug 29806: Regression tests Bug 29806: Fix POST /holds use of pickup_locations Bug 29806: Regression tests Bug 29806: Call ->as_list for ->pickup_locations on request.pl Bug 29806: Fix POST /holds use of pickup_locations Bug 29806: Regression tests Bug 29806: Call ->as_list for ->pickup_locations on request.pl Bug 29806: Fix POST /holds use of pickup_locations Bug 29806: (follow-up) Call ->as_list for ->pickup_locations on request.pl |
Description
Tomás Cohen Arazi
2022-01-06 10:45:35 UTC
I did a: $ git grep '\->pickup_locations' and my (manual) findings are: Koha/Biblio.pm: ok Koha/Biblios.pm: ok Koha/Item.pm: ok Koha/REST/V1/Biblios.pm: ok Koha/REST/V1/Holds.pm: boo Koha/Template/Plugin/Branches.pm: boo reserve/request.pl: boo t/db_dependent/Koha/Biblio.t: ok t/db_dependent/Koha/Biblios.t: ok t/db_dependent/Koha/Hold.t: ok t/db_dependent/Koha/Item.t: ok t/db_dependent/Template/Plugin/Branches.t: ok t/db_dependent/api/v1/holds.t: ok Working on fixes... Koha/Template/Plugin/Branches.pm solved on bug 29807. Created attachment 129058 [details] [review] Bug 29806: Call ->as_list for ->pickup_locations on request.pl Created attachment 129059 [details] [review] Bug 29806: Regression tests Created attachment 129060 [details] [review] Bug 29806: Fix POST /holds use of pickup_locations This patch makes Koha::Item->pickup_locations and Koha::Biblio->pickup_locations explicitly call ->as_list in list context. This way we workaround bug 28871. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail, the route gives a 500 (unhandled exception) 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass, the route correctly returns a 400 with a message about the pickup location being invalid 5. Sign off :-D Created attachment 129130 [details] [review] Bug 29806: Regression tests Signed-off-by: David Nind <david@davidnind.com> Created attachment 129131 [details] [review] Bug 29806: Call ->as_list for ->pickup_locations on request.pl Signed-off-by: David Nind <david@davidnind.com> Created attachment 129132 [details] [review] Bug 29806: Fix POST /holds use of pickup_locations This patch makes Koha::Item->pickup_locations and Koha::Biblio->pickup_locations explicitly call ->as_list in list context. This way we workaround bug 28871. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail, the route gives a 500 (unhandled exception) 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass, the route correctly returns a 400 with a message about the pickup location being invalid 5. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Created attachment 129183 [details] [review] Bug 29806: Regression tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 129184 [details] [review] Bug 29806: Call ->as_list for ->pickup_locations on request.pl Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 129185 [details] [review] Bug 29806: Fix POST /holds use of pickup_locations This patch makes Koha::Item->pickup_locations and Koha::Biblio->pickup_locations explicitly call ->as_list in list context. This way we workaround bug 28871. To test: 1. Apply the regression tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => FAIL: Tests fail, the route gives a 500 (unhandled exception) 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass, the route correctly returns a 400 with a message about the pickup location being invalid 5. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 129186 [details] [review] Bug 29806: (follow-up) Call ->as_list for ->pickup_locations on request.pl Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Hi! Thanks for the follow-up. Strictly speaking, Koha::Biblios->pickup_locations IS list context aware so this wouldn't be needed. I skipped it because of that reason, but that was before I wrote to koha-devel with my suggestion that we call ->as_list explicitly all the time. So, this patch is welcome and good to be included! (In reply to Jonathan Druart from comment #12) > Created attachment 129186 [details] [review] [review] > Bug 29806: (follow-up) Call ->as_list for ->pickup_locations on request.pl > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for 22.05, thanks to everybody involved 🦄 Pushed to 21.11.x for 21.11.03 Pushed to 21.05.x for 21.05.11 Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed. |