The Koha::{Biblio|Item}->pickup_locations methods internally use Koha::Libraries->empty, which doesn't honour list context (bug 28871). When called in list context, if the Koha::Libraries resultset was built using ->empty, then things will explode, as exposed by bug 29804.
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 [U+1F984]
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.