[Bug 42374] New: DB Upgrade from the UI is broken
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Bug ID: 42374 Summary: DB Upgrade from the UI is broken Initiative type: --- Sponsorship --- status: Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: blocker Priority: P5 - low Component: Installation and upgrade (command-line installer) Assignee: koha-bugs@lists.koha-community.org Reporter: jonathan.druart@gmail.com QA Contact: testopia@bugs.koha-community.org CC: tomascohen@gmail.com
From bug 17438 comment 17
Thats how i reproduce it: - git checkout v25.11.03-2 - reset_all - git checkout main - restart_all - open koha and run web installer -> error occurs If you run updatedatabase in shell the error is not thrown ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |38384 --- Comment #1 from Jonathan Druart <jonathan.druart@gmail.com> --- I think it's caused by bug 38384 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38384 [Bug 38384] General fix for plugins breaking database transactions -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #2 from Jonathan Druart <jonathan.druart@gmail.com> --- I can fix the issue in /usr/share/perl5/Business/ISBN/Data.pm if I replace this line: 408 if( defined $ENV{ISBN_RANGE_MESSAGE} and ! -e $ENV{ISBN_RANGE_MESSAGE} ) { with 408 if( defined $ENV{ISBN_RANGE_MESSAGE} && $ENV{ISBN_RANGE_MESSAGE} && ! -e $ENV{ISBN_RANGE_MESSAGE} ) { But it's not clear what's happening. I do not recreate when I use then require Business::ISBN in a simple perl script. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #3 from Jonathan Druart <jonathan.druart@gmail.com> --- $ENV{ISBN_RANGE_MESSAGE} is set to an empty string at some point, but why and where? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- I notice that the following entry in %ENV appears after plack.psgi loads C4::Koha and when C4::Koha loads Business::ISBN: 'ISBN_RANGE_MESSAGE' => undef, It looks like C4::Koha has been loading Business::ISBN since 2013, and plack.psgi has loaded C4::Koha since 2015. There's some weird code in Business::ISBN::Data which I'm guessing sets ISBN_RANGE_MESSAGE to undef by accident... So it's going to be something else... By the time it hits install.pl it's become this: 'ISBN_RANGE_MESSAGE' => '', I'm guessing that would be due to the CGI emulation where it takes the %ENV and turns it into the Plack $env. -- It might be bug 38384. One patch moved 'require Koha::Plugins;' to 'use Koha::Plugins;' And that creates a cascading chain of dependencies being loaded... C4::Output, C4::Auth, Koha::TwoFactorAuth, C4::Letters, C4::Members, C4::Overdues, C4::Accounts, C4::Stats, Koha::Statistics, Koha::Items, C4::Biblio, and finally C4::Koha, which calls "use Business::ISBN" again. Business::ISBN calls Business::ISBN::Data which runs this line of code: %Business::ISBN::country_data = _get_data(); And that gets us to here: carp "ISBN_RANGE_MESSAGE is set to [$ENV{ISBN_RANGE_MESSAGE}] but that file does not exist!\nTrying to use the default locations\n"; -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #4)
It might be bug 38384.
And by "might" I mean it definitely is haha. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- This is so cool... I reckon it's an autovivication bug in Business::ISBN::Data! /usr/share/perl5/Business/ISBN/Data.pm 413 no warnings 'uninitialized'; 414 my @candidates = grep { -e } ( 415 $ENV{ISBN_RANGE_MESSAGE}, # env 416 catfile( dirname( __FILE__ ), $file ), # next to the module 417 $file, # current directory 418 ); It's the grep! If you write a Perl script, you can test it by dumping %ENV before and after the following: grep { -e } $ENV{ISBN_RANGE_MESSAGE}; If you look at https://perldoc.perl.org/functions/grep: "Note that $_ is an alias to the list value, so it can be used to modify the elements of the LIST. While this is useful and supported, it can cause bizarre results if the elements of LIST are not variables." Yiiiiiikes. So I'm going to report a bug on Business::ISBN(::Data) for this one, but yeah... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- Honestly, Koha::Plugins could really use an overhaul... it loads way too much stuff for just updating the database. It would be good if someone could explain why we need "use Koha::Plugins" in updatedatabase.pl instead of just that "require Koha::Plugins" for the old update. -- A hacky fix would be to remove $ENV{ISBN_RANGE_MESSAGE} before calling Business::ISBN. That would resolve the problem, but it's ugly. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- While reporting https://github.com/briandfoy/business-isbn-data/issues/236 it got me thinking... so I started looking through CGI::Compile, CGI::Emulate::PSGI, and Plack::App::WrapCGI and nothing stood out to me. That's when I started thinking about Koha::Middleware::SetEnv. But it was clean. You know what it is? system() While the environmental variables are shared between install.pl and updatedatabase.pl, they're still different processes, and Perl's "undef" isn't meaningful in terms of OS environmental variables, so it gets stringified as "". *brainsplosion* -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #9 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #3)
$ENV{ISBN_RANGE_MESSAGE} is set to an empty string at some point, but why and where?
That was driving me crazy haha. I'm glad to have worked it out. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- So.... yeah... technically this is all a bug in Business::ISBN::Data But doing "use Koha::Plugins" in updatedatabase.pl is pretty suboptimal. But if we want to keep that... then the quick and dirty fix of just removing ISBN_RANGE_MESSAGE from %ENV is an option. I really don't like it, but it would be easy. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |dcook@prosentient.com.au |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion --- Comment #11 from David Cook <dcook@prosentient.com.au> --- I'm going to provide a patch, but I'm not sure if it's the direction we want to go or not. It fixes the immediate problem but... yeah. Curious what others think about this one. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #12 from David Cook <dcook@prosentient.com.au> --- Created attachment 197469 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197469&action=edit Bug 42374: Remove buggy ISBN_RANGE_MESSAGE env variable value This patch removes an ISBN_RANGE_MESSAGE env variable that is accidentally set by Business::ISBN::Data. Test plan: - git checkout v25.11.03-2 - reset_all - git checkout -b 42374 origin/main - Apply the patch - restart_all - Go to staff interface (e.g. http://localhost:8081/) - Log into web installer - Click "Update your database" - Note it works - Repeat the process but instead of using the web installer use updatedatabase.pl directly (say through koha-upgrade-schema) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #13 from Jonathan Druart <jonathan.druart@gmail.com> --- Good catch on finding the root cause (autovivication on grep). I was focussing on trying to replicate the problem with a simple script to report the issue upstream. I think the change I suggested in comment 2 is a valid fix for Business::ISBN::Data. Regarding our temporary fix, shouldn't we instead revert the use Koha::Plugins and replace it with a require? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #14 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 197471 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197471&action=edit Bug 42374: Restore require Koha::Plugins (instead of use) See comment 4 for a detail explanation of the problem. Test plan: hit /cgi-bin/koha/installer/install.pl login with the DB credentials upgrade => Without this patch you get ``` ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations ``` => With this patch applied: "Everything went okay. Update done." -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197471|Bug 42374: Restore require |[alternative-patch] Bug description|Koha::Plugins (instead of |42374: Restore require |use) |Koha::Plugins (instead of | |use) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #13)
Good catch on finding the root cause (autovivication on grep).
Thanks :)
I was focussing on trying to replicate the problem with a simple script to report the issue upstream.
Yeah, it was a complicated one for sure. I've reported it at https://github.com/briandfoy/business-isbn-data/issues/236 and brian d foy has fixed it in version 20260416.001 of Business::ISBN::Data. I've actually been wondering if we need to package Business::ISBN::Data in the Koha Community because I had a Koha library recently not have a 979* ISBN validated because Debian's Business::ISBN::Data was too old. I'll raise a separate issue for that, since it affects a number of things...
I think the change I suggested in comment 2 is a valid fix for Business::ISBN::Data.
You could propose that to brian d foy as well if you like. He addressed the bug a different way, but I think you make a good point there. Maybe add that to the Github issue?
Regarding our temporary fix, shouldn't we instead revert the use Koha::Plugins and replace it with a require?
If that doesn't have any other negative impacts, then I'm all for that - yes. I really don't like that "use Koha::Plugins" as it unnecessarily loads so many modules, which will just slow down the database upgrade. Why don't we do both patches? I'd really like to use your patch, as I think it's the best for Koha overall. Then we can include my patch as a "just in case". And at some point we'll have an upgraded Business::ISBN::Data without the bug as well. Can't go wrong with multiple layers hehe. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |25.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=42419 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197469|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197471|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #16 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 197764 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197764&action=edit Bug 42374: Remove buggy ISBN_RANGE_MESSAGE env variable value This patch removes an ISBN_RANGE_MESSAGE env variable that is accidentally set by Business::ISBN::Data. Test plan: - git checkout v25.11.03-2 - reset_all - git checkout -b 42374 origin/main - Apply the patch - restart_all - Go to staff interface (e.g. http://localhost:8081/) - Log into web installer - Click "Update your database" - Note it works - Repeat the process but instead of using the web installer use updatedatabase.pl directly (say through koha-upgrade-schema) Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #17 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 197765 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197765&action=edit Bug 42374: Restore require Koha::Plugins (instead of use) See comment 4 for a detail explanation of the problem. Test plan: hit /cgi-bin/koha/installer/install.pl login with the DB credentials upgrade => Without this patch you get ``` ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations ``` => With this patch applied: "Everything went okay. Update done." Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #18 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #15)
Why don't we do both patches? I'd really like to use your patch, as I think it's the best for Koha overall. Then we can include my patch as a "just in case".
Let's go with both! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|25.11 |unspecified Keywords| |roadmap_25_11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|roadmap_25_11 |rel_25_11_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #19 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #18)
(In reply to David Cook from comment #15)
Why don't we do both patches? I'd really like to use your patch, as I think it's the best for Koha overall. Then we can include my patch as a "just in case".
Let's go with both!
\o/ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |emily.lamancusa@montgomeryc |y.org |ountymd.gov CC| |emily.lamancusa@montgomeryc | |ountymd.gov -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197764|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197765|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #20 from Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 198195 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198195&action=edit Bug 42374: Remove buggy ISBN_RANGE_MESSAGE env variable value This patch removes an ISBN_RANGE_MESSAGE env variable that is accidentally set by Business::ISBN::Data. Test plan: - git checkout v25.11.03-2 - reset_all - git checkout -b 42374 origin/main - Apply the patch - restart_all - Go to staff interface (e.g. http://localhost:8081/) - Log into web installer - Click "Update your database" - Note it works - Repeat the process but instead of using the web installer use updatedatabase.pl directly (say through koha-upgrade-schema) Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #21 from Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 198196 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198196&action=edit Bug 42374: Restore require Koha::Plugins (instead of use) See comment 4 for a detail explanation of the problem. Test plan: hit /cgi-bin/koha/installer/install.pl login with the DB credentials upgrade => Without this patch you get ``` ISBN_RANGE_MESSAGE is set to [] but that file does not exist! Trying to use the default locations ``` => With this patch applied: "Everything went okay. Update done." Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #22 from Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> --- Thanks to both of you for tracking that down! These patches make sense and don't cause any regressions. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |26.05.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #23 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 26.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Jacob O'Mara <jacob.omara@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|26.05.00 |26.05.00,25.11.05 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 --- Comment #24 from Jacob O'Mara <jacob.omara@openfifth.co.uk> --- Thanks all, pushed to 25.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42374 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz Status|Pushed to stable |Needs documenting --- Comment #25 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 25.05 as it's not relevant -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org