|
Description
Victor Grousset/tuxayo
2025-08-20 19:05:53 UTC
> And when investigating, when running a test, it looked at first
> as if it failed. But no it works.
It really depends on the test.
There are tons of tests with `use Test::NoWarnings;` that blow up.
prove t/Budgets
And those who don't, calling prove on multiple test files becomes unreadable.
This is an interesting one.
Take this warning:
Attempt to call undefined import method with arguments ("get_session") via package "C4::Auth" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Search/History.pm line 5.
Line 5 is the following:
use C4::Auth qw( get_session );
At a glance, that's not the same as 'Class::That::Does::Not::Exist->import("foo");'
If we look at C4/Auth.pm, we'll see that we set up Exporter similar to how the documentation suggests on the Exporter documentation page.
That said... it could be worthwhile looking at https://perldoc.perl.org/Exporter#Playing-Safe
Given how these warnings are all coming from C4 modules, it adds weight to the idea that the issue relates to the Exporter. And I'm guessing circular dependencies... although that's just a guess at this stage. -- Looking at other reports of this online and in their cases it seems to relate more to the "use Module VERSION LIST" syntax. (In reply to Victor Grousset/tuxayo from comment #0) > == Test plan == > 1. Start a Debian 13 Koha: KOHA_IMAGE=main-trixie ktd up So I used the "ku" alias instead of "bin/ktd". I had to set the KOHA_IMAGE in my .env file. Then I kept getting this error at startup: koha-1 | Running [koha-mysql kohadev -e 'UPDATE systempreferences SET value="1" WHERE variable="RESTBasicAuth"']... koha-1 | ERROR 2026 (HY000): TLS/SSL error: SSL is required, but the server does not support it koha-1 exited with code 1 So on my next try while the container was starting up, I got onto the container and did the following to turn off SSL for the client: vi /etc/mysql/my.cnf ssl=off Once I'm on a fully started up container using KOHA_IMAGE=main-trixie, I do this simple test:
perl -MC4::Auth -e "warn 'bowwow';"
This yields many warnings:
Attempt to call undefined import method with arguments ("get_session") via package "C4::Auth" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Search/History.pm line 5.
Attempt to call undefined import method with arguments ("get_template_and_user") via package "C4::Auth" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Output.pm line 33.
Attempt to call undefined import method with arguments ("GetNormalizedISBN" ...) via package "C4::Koha" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Biblio.pm line 27.
Attempt to call undefined import method with arguments ("haspermission") via package "C4::Auth" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Serials.pm line 37.
Attempt to call undefined import method with arguments ("UpdateStats") via package "C4::Stats" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Circulation.pm line 28.
Attempt to call undefined import method with arguments ("CheckReserves" ...) via package "C4::Reserves" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Circulation.pm line 30.
Attempt to call undefined import method with arguments ("barcodedecode") via package "C4::Circulation" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Items.pm line 49.
Attempt to call undefined import method with arguments ("getitemtypeimagelocation") via package "C4::Koha" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/ItemType.pm line 20.
Attempt to call undefined import method with arguments ("GetPreparedLetter" ...) via package "C4::Letters" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Message.pm line 24.
Attempt to call undefined import method with arguments ("UpdateStats") via package "C4::Stats" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Account.pm line 28.
Attempt to call undefined import method with arguments ("GetFine") via package "C4::Overdues" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Account.pm line 29.
Attempt to call undefined import method with arguments ("checkpw_hash") via package "C4::Auth" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Patron.pm line 30.
Attempt to call undefined import method with arguments ("GetPreparedLetter" ...) via package "C4::Letters" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Patron.pm line 32.
Attempt to call undefined import method with arguments ("AddReturn") via package "C4::Circulation" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Checkouts.pm line 23.
Attempt to call undefined import method with arguments ("AddRenewal" ...) via package "C4::Circulation" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Checkout.pm line 26.
Attempt to call undefined import method with arguments ("CanBookBeIssued" ...) via package "C4::Circulation" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/CurbsidePickup.pm line 26.
Attempt to call undefined import method with arguments ("GetMessagingPreferences") via package "C4::Members::Messaging" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/CurbsidePickup.pm line 27.
Attempt to call undefined import method with arguments ("GetPreparedLetter" ...) via package "C4::Letters" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/CurbsidePickup.pm line 28.
Attempt to call undefined import method with arguments ("barcodedecode" ...) via package "C4::Circulation" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Item.pm line 29.
Attempt to call undefined import method with arguments ("GetBranchItemRule") via package "C4::Circulation" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/HoldsQueue.pm line 26.
Attempt to call undefined import method with arguments ("UpdateStats") via package "C4::Stats" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Recalls.pm line 28.
Attempt to call undefined import method with arguments ("GetPreparedLetter" ...) via package "C4::Letters" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Hold.pm line 26.
Attempt to call undefined import method with arguments ("GetAuthorisedValues") via package "C4::Koha" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/ILL/Request/Logger.pm line 23.
Attempt to call undefined import method with arguments ("CanBookBeIssued" ...) via package "C4::Circulation" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/ILL/Request.pm line 49.
Attempt to call undefined import method with arguments ("GetPreparedLetter") via package "C4::Letters" (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Members.pm line 29.
bowwow at -e line 1.
If we re-factor C4::Auth's internals to move "use C4::Context;" and all the Exporter stuff to above our Koha module loads, then we can eliminate the warnings for C4::Auth in this list.
--
This suggests that it is indeed an issue with how we setup the Exporter. If you look carefully at files like C4::Auth, the way we set up the Exporter makes no sense.
Let's look at another example...
Attempt to call undefined import method with arguments ("GetNormalizedISBN" ...) via package "C4::Koha" (Perhaps you forgot to load the package?) at /kohadevbox/koha/Koha/Biblio.pm line 27.
The 27th line of Koha::Biblio is the following:
use C4::Koha qw( GetNormalizedISBN GetNormalizedUPC GetNormalizedOCLCNumber );
We move the Exporter block up to under Modern::Perl, and we re-run this code: perl -MC4::Auth -e "warn 'bowwow';"
And sure enough the warnings for C4::Koha disappear from the list as well.
So it looks like we have some cleanup to do before supporting Debian Trixie (13) or Ubuuntu plucky (25.04)
Here is
grep -l -R "Exporter" C4/* | wc -l
66
grep -l -R "Exporter" Koha/* | wc -l
22
It looks like there might be some other problem files too...
For some bizarre reason suggestion/suggestion.pl has a "require Exporter" statement in it as well.
--
Note that not all cases will need to be fixed either. At a glance, Koha/Util/Normalize.pm looks like its Exporter setup is fine.
But all cases will need to be reviewed at the very least, although we could probably make that easier by just doing this:
find C4 Koha -name "*.pm" -exec perl -c {} \;
Anyway, hopefully that's illuminating. I've updated the "System requirements and recommendations" wiki page to say we're not recommending these latest Debian/Ubuntu OSes, and that people should double-check the supported OSes for the supported Koha versions. Recent struggles with module dependencies, imports, exports were bug 36432 and bug 35959 In case the other considered solutions and the final patches can help. also bug 36526 ----- IIUC, a way to fix would be to move BEGIN sections (eventually in rare cases splitting them) containing the exports. So moving them above our bunch of `use C4::foo`? Because one of them might transitively `use C4::the_current_module qw( plop );`. And it fails because the exports are after the `use C4::foo`. So it can finish processing `use C4::foo` without doing `use C4::the_current_module qw( plop );`. Which is not ready because the exports are not done yet. Does a fix can look like this?: https://gitlab.com/tuxayo/Koha/-/commit/8859404205a18bcfe06efe9f19a83871243d51a2 Is removes some warnings but I'm not sure if I moved stuff the right way at the best place. (In reply to Victor Grousset/tuxayo from comment #10) > Does a fix can look like this?: > https://gitlab.com/tuxayo/Koha/-/commit/ > 8859404205a18bcfe06efe9f19a83871243d51a2 > Is removes some warnings but I'm not sure if I moved stuff the right way at > the best place. Yeah that looks right to me. Created attachment 186031 [details] [review] Bug 40680: Remove duplicated code From commit b40ceb1cf5cb195f81347e090ade5e8bc3f316de Bug 39367: Manual changes Created attachment 186032 [details] [review] Bug 40680: Move EXPORT_OK right after Modern::Perl So it is done before the other use statements. Also uniformize the pattern, must now be: ``` use Modern::Perl; use base 'Exporter'; BEGIN { our @EXPORT_OK = qw(subroutine list); } ``` Created attachment 186033 [details] [review] Bug 40680: Fix C4/Patroncards/Patroncard.pm Attempt to call undefined import method with arguments (StrWidth) via package C4::Creators::PDF (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Patroncards/Patroncard.pm line 28. Created attachment 186034 [details] [review] Bug 40680: Fix acqui scripts Attempt to call undefined import method with arguments (get_active) via package Koha::Acquisition::Currencies (Perhaps you forgot to load the package?) at acqui/addorder.pl line 133. Created attachment 186035 [details] [review] Bug 40680: Fix admin/columns_settings.pl Attempt to call undefined import method with arguments (get_modules) via package C4::Utils::DataTables::TablesSettings (Perhaps you forgot to load the package?) at admin/columns_settings.pl line 8. Created attachment 186036 [details] [review] Bug 40680: Fix admin/matching-rules.pl Attempt to call undefined import method with arguments (valid_normalization_routines) via package C4::Matcher (Perhaps you forgot to load the package?) at admin/matching-rules.pl line 27. Created attachment 186037 [details] [review] Bug 40680: Fix TestBuilder perl -wc misc/devel/create_test_data.pl Attempt to call undefined import method with arguments ("schema") via package "Koha::Database" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 5. Attempt to call undefined import method with arguments ("_type") via package "Koha::Biblios" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 7. Attempt to call undefined import method with arguments ("_type") via package "Koha::Items" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 8. Created attachment 186038 [details] [review] Bug 40680: Fix svc/virtualshelves/search Attempt to call undefined import method with arguments (search) via package C4::Utils::DataTables::VirtualShelves (Perhaps you forgot to load the package?) at svc/virtualshelves/search line 8. Created attachment 186039 [details] [review] Bug 40680: Fix t/Labels* Attempt to call undefined import method with arguments (split_callnumber) via package C4::ClassSplitRoutine::LCC (Perhaps you forgot to load the package?) at t/Labels.t line 22. Created attachment 186040 [details] [review] Bug 40680: Fix C4::Calendar imports Attempt to call undefined import method with arguments (new ...) via package C4::Calendar (Perhaps you forgot to load the package?) at t/db_dependent/Hold.t line 25. Created attachment 186041 [details] [review] Bug 40680: Fix t tests Those patches fix most of the issues. Remaining ones: not ok 455 - Koha/ExternalContent/OverDrive.pm has syntax warnings. # Failed test 'Koha/ExternalContent/OverDrive.pm has syntax warnings.' # at xt/pl_valid.t line 63. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 365. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 573. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 643. not ok 2266 - opac/external/overdrive/auth.pl has syntax warnings. # Failed test 'opac/external/overdrive/auth.pl has syntax warnings.' # at xt/pl_valid.t line 63. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 365. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 573. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 643. not ok 2356 - opac/svc/overdrive has syntax warnings. # Failed test 'opac/svc/overdrive has syntax warnings.' # at xt/pl_valid.t line 63. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 365. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 573. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 643. Test plan: Startup ktd using trixie's image export DB_IMAGE=mariadb:latest; export KOHA_IMAGE=main-trixie; ktd pull && ktd down && ktd --selenium --es7 up Run prove xt/pl_valid.t The "Fix" patches remove the import of subroutines that are not exported by the modules (wrongly added by bug 17600). (In reply to Jonathan Druart from comment #23) > Those patches fix most of the issues. > > Remaining ones: > > not ok 455 - Koha/ExternalContent/OverDrive.pm has syntax warnings. > # Failed test 'Koha/ExternalContent/OverDrive.pm has syntax warnings.' > # at xt/pl_valid.t line 63. > # Possible precedence issue with control flow operator (die) at > /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 365. > # Possible precedence issue with control flow operator (die) at > /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 573. > # Possible precedence issue with control flow operator (die) at > /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 643. 365 die ($data->{message} || $data->{errorCode})."\n"; 573 die ($data->{message} || $data->{errorCode})."\n"; 643 die ($data->{message} || $data->{errorCode})."\n"; This is weird, the code looks correct. I guess perl warns because it expects to see die and "or" instead of "||", but here it's not a die ... or! (In reply to Jonathan Druart from comment #25) > (In reply to Jonathan Druart from comment #23) > > Those patches fix most of the issues. > > > > Remaining ones: > > > > not ok 455 - Koha/ExternalContent/OverDrive.pm has syntax warnings. > > # Failed test 'Koha/ExternalContent/OverDrive.pm has syntax warnings.' > > # at xt/pl_valid.t line 63. > > # Possible precedence issue with control flow operator (die) at > > /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 365. > > # Possible precedence issue with control flow operator (die) at > > /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 573. > > # Possible precedence issue with control flow operator (die) at > > /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 643. > > 365 die ($data->{message} || $data->{errorCode})."\n"; > 573 die ($data->{message} || $data->{errorCode})."\n"; > 643 die ($data->{message} || $data->{errorCode})."\n"; > > This is weird, the code looks correct. I guess perl warns because it expects > to see die and "or" instead of "||", but here it's not a die ... or! The warnings are removed with 573 my $error = ($data->{message} || $data->{errorCode})."\n"; 574 die $error; But we need to patch upstream. (In reply to Jonathan Druart from comment #26) > But we need to patch upstream. It looks like Srdjan Janković is marked as the owner on CPAN, but he disappeared from Koha/CPAN/Gitlab years ago now. I wonder if any Catalyst folks know what happened to him. I'm guessing the only way to upstream this one is having someone take over the module on CPAN. Opened an issue: https://gitlab.com/srdjan-catalyst/webservice-ils/-/issues/1 (In reply to David Cook from comment #27) > (In reply to Jonathan Druart from comment #26) > > But we need to patch upstream. > > It looks like Srdjan Janković is marked as the owner on CPAN, but he > disappeared from Koha/CPAN/Gitlab years ago now. I wonder if any Catalyst > folks know what happened to him. > > I'm guessing the only way to upstream this one is having someone take over > the module on CPAN. Catalyst will sort the access/ownership of this module. Myself or Chris will update in a few days. (In reply to Aleisha Amohia from comment #29) > Catalyst will sort the access/ownership of this module. Myself or Chris will > update in a few days. Thanks, Aleisha. Really appreciate your efforts on this one. Is this still in discussion.. what needs to happen next here? (In reply to Martin Renvoize (ashimema) from comment #31) > Is this still in discussion.. what needs to happen next here? I think that's a question for Jonathan to answer. Not sure why he's moved it to In Discussion. The patches like "Fix TestBuilder" are fascinating. It looks like there are a number of places where people have explicitly tried importing methods that were never exported in the first place. Wild! -- Since there are so many patches on this one report, I suppose we'll need to make a call of either trying to push them all ASAP or wait until after the next release. Personally, I think it's risky making so many changes all at once, especially when there are different types of changes made here. Some are fixing BEGIN blocks in .pm modules while others are fixing that non-existent import issue. But... regardless of how it's done I think we do need to push ahead. I don't think we can afford to get stuck on this one. (In reply to David Cook from comment #32) > (In reply to Martin Renvoize (ashimema) from comment #31) > > Is this still in discussion.. what needs to happen next here? > > I think that's a question for Jonathan to answer. Not sure why he's moved it > to In Discussion. As discussed in the previous comments, we are waiting for Catalyst. We need hand on the module to provide a new version. (In reply to David Cook from comment #33) > The patches like "Fix TestBuilder" are fascinating. It looks like there are > a number of places where people have explicitly tried importing methods that > were never exported in the first place. Wild! Those imports have been auto-generated (Bug 17600: Standardize our EXPORT_OK). > Since there are so many patches on this one report, I suppose we'll need to > make a call of either trying to push them all ASAP or wait until after the > next release. > > Personally, I think it's risky making so many changes all at once, > especially when there are different types of changes made here. Some are > fixing BEGIN blocks in .pm modules while others are fixing that non-existent > import issue. They are not that big and an attentive code review can help to catch issues. They are all doing fixing import/export issues. IMO it makes sense to push them all at the same time. > But... regardless of how it's done I think we do need to push ahead. I don't > think we can afford to get stuck on this one. Hence the in discussion status, it's not ready. You can already review the existing patches, the remaining work is only about WebService::ILS::OverDrive. If you prefer to move forward with the existing patches feel free to change the status. We can actually move forward, but we cannot make sure we won't introduce regressions in the meantime (people won't look at the Jenkins job if it's failing for another known reason). (In reply to Jonathan Druart from comment #34) > > Personally, I think it's risky making so many changes all at once, > > especially when there are different types of changes made here. Some are > > fixing BEGIN blocks in .pm modules while others are fixing that non-existent > > import issue. > > They are not that big and an attentive code review can help to catch issues. > They are all doing fixing import/export issues. IMO it makes sense to push > them all at the same time. (In reply to Jonathan Druart from comment #35) > If you prefer to move forward with the existing patches feel free to change > the status. We can actually move forward, but we cannot make sure we won't > introduce regressions in the meantime (people won't look at the Jenkins job > if it's failing for another known reason). Can you clarify what you mean here? It sounds to me like you're saying we should push the many changes, but that it would mean we might introduce regressions? Or am I misunderstanding? Or do you mean separate from this work that we might add regressions in unrelated patches, because we haven't moved to the newer OSes yet? (In reply to David Cook from comment #36) > (In reply to Jonathan Druart from comment #34) > > > Personally, I think it's risky making so many changes all at once, > > > especially when there are different types of changes made here. Some are > > > fixing BEGIN blocks in .pm modules while others are fixing that non-existent > > > import issue. > > > > They are not that big and an attentive code review can help to catch issues. > > They are all doing fixing import/export issues. IMO it makes sense to push > > them all at the same time. > > (In reply to Jonathan Druart from comment #35) > > If you prefer to move forward with the existing patches feel free to change > > the status. We can actually move forward, but we cannot make sure we won't > > introduce regressions in the meantime (people won't look at the Jenkins job > > if it's failing for another known reason). > > Can you clarify what you mean here? > > It sounds to me like you're saying we should push the many changes, but that > it would mean we might introduce regressions? Or am I misunderstanding? > > Or do you mean separate from this work that we might add regressions in > unrelated patches, because we haven't moved to the newer OSes yet? By "regressions" I meant "new problematic occurrences". The original goal here is to fix xt/pl_valid.t on D13. Without all the perl files fixed, the job will still fail. And nobody will care if new occurrences are pushed. But forget that. I agree with you: we don't need to wait and we should push what has been fixed already. Created attachment 186654 [details] [review] Bug 40680: Remove duplicated code From commit b40ceb1cf5cb195f81347e090ade5e8bc3f316de Bug 39367: Manual changes Created attachment 186655 [details] [review] Bug 40680: Move EXPORT_OK right after Modern::Perl So it is done before the other use statements. Also uniformize the pattern, must now be: ``` use Modern::Perl; use base 'Exporter'; BEGIN { our @EXPORT_OK = qw(subroutine list); } ``` Created attachment 186656 [details] [review] Bug 40680: Fix C4/Patroncards/Patroncard.pm Attempt to call undefined import method with arguments (StrWidth) via package C4::Creators::PDF (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Patroncards/Patroncard.pm line 28. Created attachment 186657 [details] [review] Bug 40680: Fix acqui scripts Attempt to call undefined import method with arguments (get_active) via package Koha::Acquisition::Currencies (Perhaps you forgot to load the package?) at acqui/addorder.pl line 133. Created attachment 186658 [details] [review] Bug 40680: Fix admin/columns_settings.pl Attempt to call undefined import method with arguments (get_modules) via package C4::Utils::DataTables::TablesSettings (Perhaps you forgot to load the package?) at admin/columns_settings.pl line 8. Created attachment 186659 [details] [review] Bug 40680: Fix admin/matching-rules.pl Attempt to call undefined import method with arguments (valid_normalization_routines) via package C4::Matcher (Perhaps you forgot to load the package?) at admin/matching-rules.pl line 27. Created attachment 186660 [details] [review] Bug 40680: Fix TestBuilder perl -wc misc/devel/create_test_data.pl Attempt to call undefined import method with arguments ("schema") via package "Koha::Database" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 5. Attempt to call undefined import method with arguments ("_type") via package "Koha::Biblios" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 7. Attempt to call undefined import method with arguments ("_type") via package "Koha::Items" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 8. Created attachment 186661 [details] [review] Bug 40680: Fix svc/virtualshelves/search Attempt to call undefined import method with arguments (search) via package C4::Utils::DataTables::VirtualShelves (Perhaps you forgot to load the package?) at svc/virtualshelves/search line 8. Created attachment 186662 [details] [review] Bug 40680: Fix t/Labels* Attempt to call undefined import method with arguments (split_callnumber) via package C4::ClassSplitRoutine::LCC (Perhaps you forgot to load the package?) at t/Labels.t line 22. Created attachment 186663 [details] [review] Bug 40680: Fix C4::Calendar imports Attempt to call undefined import method with arguments (new ...) via package C4::Calendar (Perhaps you forgot to load the package?) at t/db_dependent/Hold.t line 25. Created attachment 186664 [details] [review] Bug 40680: Fix t tests Created attachment 187122 [details] [review] Bug 40680: Remove duplicated code From commit b40ceb1cf5cb195f81347e090ade5e8bc3f316de Bug 39367: Manual changes Created attachment 187123 [details] [review] Bug 40680: Move EXPORT_OK right after Modern::Perl So it is done before the other use statements. Also uniformize the pattern, must now be: ``` use Modern::Perl; use base 'Exporter'; BEGIN { our @EXPORT_OK = qw(subroutine list); } ``` Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Bug38936 exported redirect_if_opac_suppressed Created attachment 187124 [details] [review] Bug 40680: Fix C4/Patroncards/Patroncard.pm Attempt to call undefined import method with arguments (StrWidth) via package C4::Creators::PDF (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Patroncards/Patroncard.pm line 28. Created attachment 187125 [details] [review] Bug 40680: Fix acqui scripts Attempt to call undefined import method with arguments (get_active) via package Koha::Acquisition::Currencies (Perhaps you forgot to load the package?) at acqui/addorder.pl line 133. Created attachment 187126 [details] [review] Bug 40680: Fix admin/columns_settings.pl Attempt to call undefined import method with arguments (get_modules) via package C4::Utils::DataTables::TablesSettings (Perhaps you forgot to load the package?) at admin/columns_settings.pl line 8. Created attachment 187127 [details] [review] Bug 40680: Fix admin/matching-rules.pl Attempt to call undefined import method with arguments (valid_normalization_routines) via package C4::Matcher (Perhaps you forgot to load the package?) at admin/matching-rules.pl line 27. Created attachment 187128 [details] [review] Bug 40680: Fix TestBuilder perl -wc misc/devel/create_test_data.pl Attempt to call undefined import method with arguments ("schema") via package "Koha::Database" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 5. Attempt to call undefined import method with arguments ("_type") via package "Koha::Biblios" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 7. Attempt to call undefined import method with arguments ("_type") via package "Koha::Items" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 8. Created attachment 187129 [details] [review] Bug 40680: Fix svc/virtualshelves/search Attempt to call undefined import method with arguments (search) via package C4::Utils::DataTables::VirtualShelves (Perhaps you forgot to load the package?) at svc/virtualshelves/search line 8. Created attachment 187130 [details] [review] Bug 40680: Fix t/Labels* Attempt to call undefined import method with arguments (split_callnumber) via package C4::ClassSplitRoutine::LCC (Perhaps you forgot to load the package?) at t/Labels.t line 22. Created attachment 187131 [details] [review] Bug 40680: Fix C4::Calendar imports Attempt to call undefined import method with arguments (new ...) via package C4::Calendar (Perhaps you forgot to load the package?) at t/db_dependent/Hold.t line 25. Created attachment 187132 [details] [review] Bug 40680: Fix t tests Rebased (Bug 38936 exported redirect_if_opac_suppressed) xt/pl_valid.t ok except the expected Koha/ExternalContent/OverDrive.pm opac/external/overdrive/auth.pl opac/svc/overdrive Even if webservice-ils is sorted out, there are still a lot of warnings related to imports when running tests (found with --run-light-test-suite) on Debian 13:
t/db_dependent/Koha/Plugins/Account_hooks.t
t/db_dependent/Koha/Template/Plugin/JSConsents.t
t/db_dependent/Matcher.t
t/db_dependent/Search/History.t
t/db_dependent/Koha/Template/Plugin/TablesSettings.t
t/db_dependent/Barcodes_ValueBuilder.t
t/db_dependent/SocialData.t
t/db_dependent/SIP/SIPServer.t
t/db_dependent/Labels/t_Batch.t
t/db_dependent/Holidays.t
t/db_dependent/Holds.t
t/db_dependent/Koha/Template/Plugin/KohaPlugins.t
t/db_dependent/Barcodes.t
t/db_dependent/Heading_MARC21.t
t/db_dependent/Heading.t
t/db_dependent/ILSDI_Services.t
t/db_dependent/UsageStats.t
t/Labels_split_Regex.t
t/ClassSortRoutine_Dewey.t
t/ClassSortRoutine_LCC.t
t/AuthoritiesMarc_MARC21.t
t/Search/History.t
t/ClassSortRoutine_Generic.t
t/Labels.t
t/Labels_split_ddcn.t
xt/author/pod_coverage.t
t/Creators.t
t/AuthoritiesMarc_UNIMARC.t
t/SMS.t
It seems to be another issues than imports but still something that pops with perl 5.40 warning policy:
t/db_dependent/Budgets.t
# Use of uninitialized value $row[0] in hash element at /usr/lib/x86_64-linux-gnu/perl5/5.40/DBI.pm line 2122.
# at /usr/lib/x86_64-linux-gnu/perl5/5.40/DBI.pm line 2122.
# DBD::_::st::fetchall_hashref(DBI::st=HASH(0x55f6b18391d8), "budget_id") called at /usr/lib/x86_64-linux-gnu/perl5/5.40/DBD/mysql.pm line 866
# DBD::mysql::st::__ANON__(DBI::st=HASH(0x55f6b18391d8), "budget_id") called at /usr/lib/x86_64-linux-gnu/perl5/5.40/DBI.pm line 1684
# DBD::_::db::selectall_hashref(DBI::db=HASH(0x55f6b10f7578), "\x{a} SELECT shipmentcost_budgetid as budget_id,\x{a} "..., "budget_id") called at /kohadevbox/koha/C4/Budgets.pm line 750
# C4::Budgets::GetBudgetHierarchy(290) called at /kohadevbox/koha/C4/Budgets.pm line 1442
# C4::Budgets::CloneBudgetPeriod(HASH(0x55f6b180f520)) called at t/db_dependent/Budgets.t line 598
t/db_dependent/Reports/Guided.t
# There were 1 warning(s)
# Previous test 5 ''
# Use of uninitialized value in pattern match (m//) at t/db_dependent/Reports/Guided.t line 631.
# at t/db_dependent/Reports/Guided.t line 631.
# main::__ANON__() called at /usr/share/perl/5.40/Test/Builder.pm line 374
# eval {...} called at /usr/share/perl/5.40/Test/Builder.pm line 374
# Test::Builder::subtest(Test::Builder=HASH(0x55c7587d0c98), "get_columns", CODE(0x55c762ab1df8)) called at /usr/share/perl/5.40/Test/More.pm line 831
# Test::More::subtest("get_columns", CODE(0x55c762ab1df8)) called at t/db_dependent/Reports/Guided.t line 641
Should they be in separate tickets?
(In reply to Victor Grousset/tuxayo from comment #61) > Should they be in separate tickets? Yes, I think so. We should not wait before pushing this first set, it will be boring to rebase... Created attachment 187385 [details] [review] Bug 40680: Fix t tests (In reply to Victor Grousset/tuxayo from comment #61) > Even if webservice-ils is sorted out, there are still a lot of warnings > related to imports when running tests (found with --run-light-test-suite) on > Debian 13: > > t/db_dependent/Koha/Plugins/Account_hooks.t > t/db_dependent/Koha/Template/Plugin/JSConsents.t > t/db_dependent/Matcher.t > t/db_dependent/Search/History.t > t/db_dependent/Koha/Template/Plugin/TablesSettings.t > t/db_dependent/Barcodes_ValueBuilder.t > t/db_dependent/SocialData.t > t/db_dependent/SIP/SIPServer.t > t/db_dependent/Labels/t_Batch.t > t/db_dependent/Holidays.t > t/db_dependent/Holds.t > t/db_dependent/Koha/Template/Plugin/KohaPlugins.t > t/db_dependent/Barcodes.t > t/db_dependent/Heading_MARC21.t > t/db_dependent/Heading.t > t/db_dependent/ILSDI_Services.t > t/db_dependent/UsageStats.t > t/Labels_split_Regex.t > t/ClassSortRoutine_Dewey.t > t/ClassSortRoutine_LCC.t > t/AuthoritiesMarc_MARC21.t > t/Search/History.t > t/ClassSortRoutine_Generic.t > t/Labels.t > t/Labels_split_ddcn.t > xt/author/pod_coverage.t > t/Creators.t > t/AuthoritiesMarc_UNIMARC.t > t/SMS.t Fixed in the last patch. xt/author/pod_coverage.t is still failing because of Overdrive. (In reply to Jonathan Druart from comment #64) > xt/author/pod_coverage.t is still failing because of Overdrive. I haven't heard any news from Catalyst about a new maintainer taking over WebService::ILS from Srdjan, so I wonder if we should copy it into the "lib/" directory for now. It's not ideal from a long-term perspective, but it could be an interim solution. It looks like MetaCPAN is down at the moment, so I'll have to take a look later at just how many modules that would involve. (In reply to David Cook from comment #65) > (In reply to Jonathan Druart from comment #64) > > xt/author/pod_coverage.t is still failing because of Overdrive. > > I haven't heard any news from Catalyst about a new maintainer taking over > WebService::ILS from Srdjan, so I wonder if we should copy it into the > "lib/" directory for now. It's not ideal from a long-term perspective, but > it could be an interim solution. > > It looks like MetaCPAN is down at the moment, so I'll have to take a look > later at just how many modules that would involve. Yes sorry team, we have followed up but not heard back from Srdjan. We should look to fix this in the meantime until a more permanent solution can be done. (In reply to Aleisha Amohia from comment #66) > (In reply to David Cook from comment #65) > > (In reply to Jonathan Druart from comment #64) > > > xt/author/pod_coverage.t is still failing because of Overdrive. > > > > I haven't heard any news from Catalyst about a new maintainer taking over > > WebService::ILS from Srdjan, so I wonder if we should copy it into the > > "lib/" directory for now. It's not ideal from a long-term perspective, but > > it could be an interim solution. > > > > It looks like MetaCPAN is down at the moment, so I'll have to take a look > > later at just how many modules that would involve. > > Yes sorry team, we have followed up but not heard back from Srdjan. We > should look to fix this in the meantime until a more permanent solution can > be done. Thanks for the follow-up Aleisha. Since WebService::ILS is technically unmaintained, we could permanently fork it or write new Koha modules for it. At a glance, I think we're only using WebService::ILS::OverDrive::Patron although looking through /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm it looks like there's a bunch of subclassing so there's a number of modules used... so rewriting might be overly burdensome. The warnings for Perl 5.40 seem to only apply to WebService::ILS::OverDrive::Patron so we could be cheeky and just copy that file into "lib/" and hope for the best too, although probably best to copy over the whole of "WebService::ILS" to be on the safe side. It might be enough to subclass /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm and then override the methods that we need to fix for Perl 5.40 too. Not a brilliant long-term solution but a practical short-term one. -- It looks like the "RecordedBooks" integration was removed in 23.11, so if we did want to fork and do own version in Koha I guess we would just need to fork the WebService/ILS.pm, WebService/ILS/JSON.pm, and WebService/ILS/OverDrive* modules. I decided to take another look at CPAN to see how you can take over a module on CPAN without the maintainer's permission... On https://www.cpan.org/misc/cpan-faq.html the relevant sections appear to be "How do I go about maintaining a module when the author is unresponsive?" and "How do I adopt or take over a module already on CPAN?". If we want to adopt the module, we'll want 1 person to make contact with the PAUSE admins. It might be best if this is Chris Cormack since I think he's the one who has been trying to contact Srdjan. He'd probably be able to make the most compelling case. In the meantime, CPAN suggests: "If you need changes made immediately, consider applying your patches to the current module, changing the version and requiring that version for your application. Eventually the author will turn up and apply your patches, offer you maintenance of the module or, if the author doesn't respond in a year, you may get maintenance by having interest." Or "If you need changes in order for another module or application to work, consider making the needed changes and bundling the new version with your own distribution and noting the change well in the documentation. Do not upload the new version under the same namespace to CPAN until the matter has been resolved with the author or CPAN." I think the latter is probably more likely and reflects what I was saying above? (In reply to David Cook from comment #68) > It might be enough to subclass > /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm and then override the > methods that we need to fix for Perl 5.40 too. > > Not a brilliant long-term solution but a practical short-term one. Since it's wrapped in Koha/ExternalContent/OverDrive.pm , the two only relevant uses are there and in t/00-load.t . So that's not an invasive workaround. Looks good. Created attachment 187609 [details] [review] Bug 40680: Remove duplicated code From commit b40ceb1cf5cb195f81347e090ade5e8bc3f316de Bug 39367: Manual changes Created attachment 187610 [details] [review] Bug 40680: Move EXPORT_OK right after Modern::Perl So it is done before the other use statements. Also uniformize the pattern, must now be: ``` use Modern::Perl; use base 'Exporter'; BEGIN { our @EXPORT_OK = qw(subroutine list); } ``` Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Bug38936 exported redirect_if_opac_suppressed Created attachment 187611 [details] [review] Bug 40680: Fix C4/Patroncards/Patroncard.pm Attempt to call undefined import method with arguments (StrWidth) via package C4::Creators::PDF (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Patroncards/Patroncard.pm line 28. Created attachment 187612 [details] [review] Bug 40680: Fix acqui scripts Attempt to call undefined import method with arguments (get_active) via package Koha::Acquisition::Currencies (Perhaps you forgot to load the package?) at acqui/addorder.pl line 133. Created attachment 187613 [details] [review] Bug 40680: Fix admin/columns_settings.pl Attempt to call undefined import method with arguments (get_modules) via package C4::Utils::DataTables::TablesSettings (Perhaps you forgot to load the package?) at admin/columns_settings.pl line 8. Created attachment 187614 [details] [review] Bug 40680: Fix admin/matching-rules.pl Attempt to call undefined import method with arguments (valid_normalization_routines) via package C4::Matcher (Perhaps you forgot to load the package?) at admin/matching-rules.pl line 27. Created attachment 187615 [details] [review] Bug 40680: Fix TestBuilder perl -wc misc/devel/create_test_data.pl Attempt to call undefined import method with arguments ("schema") via package "Koha::Database" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 5. Attempt to call undefined import method with arguments ("_type") via package "Koha::Biblios" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 7. Attempt to call undefined import method with arguments ("_type") via package "Koha::Items" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 8. Created attachment 187616 [details] [review] Bug 40680: Fix svc/virtualshelves/search Attempt to call undefined import method with arguments (search) via package C4::Utils::DataTables::VirtualShelves (Perhaps you forgot to load the package?) at svc/virtualshelves/search line 8. Created attachment 187617 [details] [review] Bug 40680: Fix t/Labels* Attempt to call undefined import method with arguments (split_callnumber) via package C4::ClassSplitRoutine::LCC (Perhaps you forgot to load the package?) at t/Labels.t line 22. Created attachment 187618 [details] [review] Bug 40680: Fix C4::Calendar imports Attempt to call undefined import method with arguments (new ...) via package C4::Calendar (Perhaps you forgot to load the package?) at t/db_dependent/Hold.t line 25. Created attachment 187619 [details] [review] Bug 40680: Fix t tests Amended "Bug 40680: Move EXPORT_OK right after Modern::Perl": add 'use Modern::Perl;' to C4/Members/Messaging.pm in (In reply to Jonathan Druart from comment #25) > 365 die ($data->{message} || $data->{errorCode})."\n"; > 573 die ($data->{message} || $data->{errorCode})."\n"; > 643 die ($data->{message} || $data->{errorCode})."\n"; > > This is weird, the code looks correct. I guess perl warns because it expects > to see die and "or" instead of "||", but here it's not a die ... or! Jonathan saved everyone else a lot of text from me by messaging me about this. After doing some reading and experimenting, I think I managed to work out the problem here. In Koha and in Perl, we often omit parentheses for functions like print, warn, die, etc. But Perl will also ignore whitespace. So the two following statements are equivalent: die ($data->{message} || $data->{errorCode})."\n"; die($data->{message} || $data->{errorCode})."\n"; Obviously the second one isn't conceptually valid. I would expect a fatal syntax error. But older Perls will just say "Useless use of concatenation (.) or string in void context" whereas now with newer Perls we get "Possible precedence issue with control flow operator" Which makes sense because that concatenation operator "." is never going to be reached. die() will always return before it gets there. So the simplest solution is just to be more explicit about the precedence we want: die(($data->{message} || $data->{errorCode})."\n"); That one is still failing, I missed it in the 1st run among the many failures:
prove t/db_dependent/Realtime_holdsqueue.t
t/db_dependent/Realtime_holdsqueue.t .. 1/2
# Failed test 'no warnings'
# at /usr/share/perl/5.40/Test/Builder.pm line 193.
# There were 1 warning(s)
# Previous test 0 ''
# Attempt to call undefined import method with arguments ("do_checkin") via package "C4::SIP::ILS::Transaction::Checkin" (Perhaps you forgot to load the package?) at t/db_dependent/Realtime_holdsqueue.t line 26.
# at t/db_dependent/Realtime_holdsqueue.t line 26.
# main::BEGIN() called at t/db_dependent/Realtime_holdsqueue.t line 26
# eval {...} called at t/db_dependent/Realtime_holdsqueue.t line 26
Besides that one, these tests were run and pass
--run-selenium-tests-only
--run-light-test-suite
Created attachment 187815 [details] [review] Bug 40680: Remove duplicated code From commit b40ceb1cf5cb195f81347e090ade5e8bc3f316de Bug 39367: Manual changes Created attachment 187816 [details] [review] Bug 40680: Move EXPORT_OK right after Modern::Perl So it is done before the other use statements. Also uniformize the pattern, must now be: ``` use Modern::Perl; use base 'Exporter'; BEGIN { our @EXPORT_OK = qw(subroutine list); } ``` Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Bug38936 exported redirect_if_opac_suppressed Created attachment 187817 [details] [review] Bug 40680: Fix C4/Patroncards/Patroncard.pm Attempt to call undefined import method with arguments (StrWidth) via package C4::Creators::PDF (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Patroncards/Patroncard.pm line 28. Created attachment 187818 [details] [review] Bug 40680: Fix acqui scripts Attempt to call undefined import method with arguments (get_active) via package Koha::Acquisition::Currencies (Perhaps you forgot to load the package?) at acqui/addorder.pl line 133. Created attachment 187819 [details] [review] Bug 40680: Fix admin/columns_settings.pl Attempt to call undefined import method with arguments (get_modules) via package C4::Utils::DataTables::TablesSettings (Perhaps you forgot to load the package?) at admin/columns_settings.pl line 8. Created attachment 187820 [details] [review] Bug 40680: Fix admin/matching-rules.pl Attempt to call undefined import method with arguments (valid_normalization_routines) via package C4::Matcher (Perhaps you forgot to load the package?) at admin/matching-rules.pl line 27. Created attachment 187821 [details] [review] Bug 40680: Fix TestBuilder perl -wc misc/devel/create_test_data.pl Attempt to call undefined import method with arguments ("schema") via package "Koha::Database" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 5. Attempt to call undefined import method with arguments ("_type") via package "Koha::Biblios" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 7. Attempt to call undefined import method with arguments ("_type") via package "Koha::Items" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 8. Created attachment 187822 [details] [review] Bug 40680: Fix svc/virtualshelves/search Attempt to call undefined import method with arguments (search) via package C4::Utils::DataTables::VirtualShelves (Perhaps you forgot to load the package?) at svc/virtualshelves/search line 8. Created attachment 187823 [details] [review] Bug 40680: Fix t/Labels* Attempt to call undefined import method with arguments (split_callnumber) via package C4::ClassSplitRoutine::LCC (Perhaps you forgot to load the package?) at t/Labels.t line 22. Created attachment 187824 [details] [review] Bug 40680: Fix C4::Calendar imports Attempt to call undefined import method with arguments (new ...) via package C4::Calendar (Perhaps you forgot to load the package?) at t/db_dependent/Hold.t line 25. Created attachment 187825 [details] [review] Bug 40680: Fix t tests Amended patch @ t/db_dependent/Realtime_holdsqueue.t:26 @ use Test::MockModule; -use C4::SIP::ILS::Transaction::Checkin qw( do_checkin ); +use C4::SIP::ILS::Transaction::Checkin; Thanks again for all the fixes :) xt/perltidy.t starts failing with the patches I think all other t & xt tests are good. Including selenium, www, ES. I still got cypress tests to rerun to sort out random failures and actual ones. And if they are due to the patches. Created attachment 187948 [details] [review] Bug 40680: Remove duplicated code From commit b40ceb1cf5cb195f81347e090ade5e8bc3f316de Bug 39367: Manual changes Created attachment 187949 [details] [review] Bug 40680: Move EXPORT_OK right after Modern::Perl So it is done before the other use statements. Also uniformize the pattern, must now be: ``` use Modern::Perl; use base 'Exporter'; BEGIN { our @EXPORT_OK = qw(subroutine list); } ``` Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Bug38936 exported redirect_if_opac_suppressed Created attachment 187950 [details] [review] Bug 40680: Fix C4/Patroncards/Patroncard.pm Attempt to call undefined import method with arguments (StrWidth) via package C4::Creators::PDF (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Patroncards/Patroncard.pm line 28. Created attachment 187951 [details] [review] Bug 40680: Fix acqui scripts Attempt to call undefined import method with arguments (get_active) via package Koha::Acquisition::Currencies (Perhaps you forgot to load the package?) at acqui/addorder.pl line 133. Created attachment 187952 [details] [review] Bug 40680: Fix admin/columns_settings.pl Attempt to call undefined import method with arguments (get_modules) via package C4::Utils::DataTables::TablesSettings (Perhaps you forgot to load the package?) at admin/columns_settings.pl line 8. Created attachment 187953 [details] [review] Bug 40680: Fix admin/matching-rules.pl Attempt to call undefined import method with arguments (valid_normalization_routines) via package C4::Matcher (Perhaps you forgot to load the package?) at admin/matching-rules.pl line 27. Created attachment 187954 [details] [review] Bug 40680: Fix TestBuilder perl -wc misc/devel/create_test_data.pl Attempt to call undefined import method with arguments ("schema") via package "Koha::Database" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 5. Attempt to call undefined import method with arguments ("_type") via package "Koha::Biblios" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 7. Attempt to call undefined import method with arguments ("_type") via package "Koha::Items" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 8. Created attachment 187955 [details] [review] Bug 40680: Fix svc/virtualshelves/search Attempt to call undefined import method with arguments (search) via package C4::Utils::DataTables::VirtualShelves (Perhaps you forgot to load the package?) at svc/virtualshelves/search line 8. Created attachment 187956 [details] [review] Bug 40680: Fix t/Labels* Attempt to call undefined import method with arguments (split_callnumber) via package C4::ClassSplitRoutine::LCC (Perhaps you forgot to load the package?) at t/Labels.t line 22. Created attachment 187957 [details] [review] Bug 40680: Fix C4::Calendar imports Attempt to call undefined import method with arguments (new ...) via package C4::Calendar (Perhaps you forgot to load the package?) at t/db_dependent/Hold.t line 25. Created attachment 187958 [details] [review] Bug 40680: Fix t tests Amended patch @ t/db_dependent/Realtime_holdsqueue.t:26 @ use Test::MockModule; -use C4::SIP::ILS::Transaction::Checkin qw( do_checkin ); +use C4::SIP::ILS::Transaction::Checkin; Created attachment 188126 [details] [review] Bug 40680: Remove duplicated code From commit b40ceb1cf5cb195f81347e090ade5e8bc3f316de Bug 39367: Manual changes Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188127 [details] [review] Bug 40680: Move EXPORT_OK right after Modern::Perl So it is done before the other use statements. Also uniformize the pattern, must now be: ``` use Modern::Perl; use base 'Exporter'; BEGIN { our @EXPORT_OK = qw(subroutine list); } ``` Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Bug38936 exported redirect_if_opac_suppressed Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188128 [details] [review] Bug 40680: Fix C4/Patroncards/Patroncard.pm Attempt to call undefined import method with arguments (StrWidth) via package C4::Creators::PDF (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Patroncards/Patroncard.pm line 28. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188129 [details] [review] Bug 40680: Fix acqui scripts Attempt to call undefined import method with arguments (get_active) via package Koha::Acquisition::Currencies (Perhaps you forgot to load the package?) at acqui/addorder.pl line 133. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188130 [details] [review] Bug 40680: Fix admin/columns_settings.pl Attempt to call undefined import method with arguments (get_modules) via package C4::Utils::DataTables::TablesSettings (Perhaps you forgot to load the package?) at admin/columns_settings.pl line 8. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188131 [details] [review] Bug 40680: Fix admin/matching-rules.pl Attempt to call undefined import method with arguments (valid_normalization_routines) via package C4::Matcher (Perhaps you forgot to load the package?) at admin/matching-rules.pl line 27. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188132 [details] [review] Bug 40680: Fix TestBuilder perl -wc misc/devel/create_test_data.pl Attempt to call undefined import method with arguments ("schema") via package "Koha::Database" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 5. Attempt to call undefined import method with arguments ("_type") via package "Koha::Biblios" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 7. Attempt to call undefined import method with arguments ("_type") via package "Koha::Items" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 8. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188133 [details] [review] Bug 40680: Fix svc/virtualshelves/search Attempt to call undefined import method with arguments (search) via package C4::Utils::DataTables::VirtualShelves (Perhaps you forgot to load the package?) at svc/virtualshelves/search line 8. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188134 [details] [review] Bug 40680: Fix t/Labels* Attempt to call undefined import method with arguments (split_callnumber) via package C4::ClassSplitRoutine::LCC (Perhaps you forgot to load the package?) at t/Labels.t line 22. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188135 [details] [review] Bug 40680: Fix C4::Calendar imports Attempt to call undefined import method with arguments (new ...) via package C4::Calendar (Perhaps you forgot to load the package?) at t/db_dependent/Hold.t line 25. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 188136 [details] [review] Bug 40680: Fix t tests Amended patch @ t/db_dependent/Realtime_holdsqueue.t:26 @ use Test::MockModule; -use C4::SIP::ILS::Transaction::Checkin qw( do_checkin ); +use C4::SIP::ILS::Transaction::Checkin; Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Thanks for the fix! :) Beside some local failures, I think all tests pass with the patches. Basic circulation works, is there anything else to test? qa script fails with tidy issues on perl files but that only happens on Debian 13. Something like the default of perltidy have changed? Anyway, that's another issue >_< My previous report of tidy failures might have been due to running on Debian 12. And Jonathan made the patches on Debian 13. Or some other confusion like that. At least now the current state is that the code is considered tidy on Debian 12. (lucky that it's not the reverse) (In reply to David Cook from comment #4) > (In reply to Victor Grousset/tuxayo from comment #0) > > == Test plan == > > 1. Start a Debian 13 Koha: KOHA_IMAGE=main-trixie ktd up > > So I used the "ku" alias instead of "bin/ktd". I had to set the KOHA_IMAGE > in my .env file. > > Then I kept getting this error at startup: > koha-1 | Running [koha-mysql kohadev -e 'UPDATE systempreferences SET > value="1" WHERE variable="RESTBasicAuth"']... > koha-1 | ERROR 2026 (HY000): TLS/SSL error: SSL is required, but the > server does not support it > koha-1 exited with code 1 > > So on my next try while the container was starting up, I got onto the > container and did the following to turn off SSL for the client: > > vi /etc/mysql/my.cnf > ssl=off Please look at bug 41099. I think keeping the current behavior is the way to go. I already patched KTD (adding the my.cnf line) so we start testing Debian 13 and Ubuntu 25.04 as people are already trying Koha there. why is this stalling exactly? (In reply to Jonathan Druart from comment #122) > why is this stalling exactly? For me, I think I'm just intimidated by the amount of changes, so I'm not sure how/where I can help. For instance, I'm not sure what the next step for this report is? I would tend to postpone this over the 25.11 release now. We are not yet advertizing Debian 13 as a supported OS. But obviously, we should fix this soon. Those patches fix tests basically so running the tests and confirming that they now pass will already give you a good idea of the sanity of the changes. Then you can test different areas in Koha. Also reading the diff, the changes are pretty trivial. I won't rebase this for months so people should get involved now... Removed dependency to bug 41099. It's not a real dependency. The issue it addresses for testing can be circumvented using DB_IMAGE=mariadb:11.4 or later. (In reply to Marcel de Rooy from comment #124) > I would tend to postpone this over the 25.11 release now. We are not yet > advertizing Debian 13 as a supported OS. But obviously, we should fix this > soon. Given that there will still be people who will not follow the system requirements (which is a pain!), won't we get bothered with more reports over the next six months if this isn't merged? It's certain there will be some issues without this. But with this merged, the risk is some weird dependency mess that didn't make enough warnings to be noticed during the tests. And didn't make a test fail (there sure are blindspot). **Which bet is safer?** Of course without anyone running Koha on Debian 13 the choice would be easy. Wait, the big assumption is that it will not be possible to backport to from main to 25.11.x. But maybe it's safe enough. Like if that's the 1st thing merged to main after 25.11.00, then does it feel safe to backport to 25.11.01 if nothing is found in 3 weeks? It would definitely be very nice if the fixes were merged for 25.11... Unfortunately it seems I made the mistake of assuming that "trixie" being in the repos = it's supported and supposed to work (which it actually does for the most part, aside from these warnings) (In reply to Michał from comment #128) > It would definitely be very nice if the fixes were merged for 25.11... > Unfortunately it seems I made the mistake of assuming that "trixie" being in > the repos = it's supported and supposed to work (which it actually does for > the most part, aside from these warnings) There are other know issues, see bug 41077. Comment on attachment 188127 [details] [review] Bug 40680: Move EXPORT_OK right after Modern::Perl Review of attachment 188127 [details] [review]: ----------------------------------------------------------------- ::: C4/Auth_with_shibboleth.pm @@ +155,4 @@ > module => 'members', > letter_code => 'WELCOME', > branchcode => $patron->branchcode, > + Good catch here with the double ,, and another instance of the same. Ew that these things compiled <.< ::: C4/Members/Messaging.pm @@ +25,5 @@ > + our @EXPORT_OK = qw( > + GetMessagingPreferences > + SetMessagingPreference > + GetMessagingOptions > + SetMessagingPreferencesFromDefaults In the whole patchset, that's the only change for that file. Is that expected that there was no proper export before? Introducing it should not cause any issues to the current callers of these methods, right? I won't work more on this until people are ready to QA and push it. (In reply to Victor Grousset/tuxayo from comment #130) > Comment on attachment 188127 [details] [review] [review] > Bug 40680: Move EXPORT_OK right after Modern::Perl > ::: C4/Members/Messaging.pm > @@ +25,5 @@ > > + our @EXPORT_OK = qw( > > + GetMessagingPreferences > > + SetMessagingPreference > > + GetMessagingOptions > > + SetMessagingPreferencesFromDefaults > > In the whole patchset, that's the only change for that file. Is that > expected that there was no proper export before? Introducing it should not > cause any issues to the current callers of these methods, right? I've noted at least those ones. So either we adjust the callers or we export, it seems that I've decided to add the exports. Koha/CurbsidePickup.pm:use C4::Members::Messaging qw( GetMessagingPreferences ); opac/opac-memberentry.pl:use C4::Members::Messaging qw( SetMessagingPreferencesFromDefaults ); t/db_dependent/Circulation.t:use C4::Members::Messaging qw( SetMessagingPreference ); Thanks for confirming that's indeed not an issue. I'm reviewing as much as I can with my knowledge. And from what Lucas told me they feel comfortable with merging this at the very start of 26.05 cycle. So I plan to finish proofreading, get a second opinion on QA chat about confirming that the base change template applied to all files is good and it should be good to go :) Surprisingly it hasn't conflicted for a while with main :D Created attachment 189875 [details] [review] Bug 40680: Remove duplicated code From commit b40ceb1cf5cb195f81347e090ade5e8bc3f316de Bug 39367: Manual changes Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189876 [details] [review] Bug 40680: Move EXPORT_OK right after Modern::Perl So it is done before the other use statements. Also uniformize the pattern, must now be: ``` use Modern::Perl; use base 'Exporter'; BEGIN { our @EXPORT_OK = qw(subroutine list); } ``` Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net> Bug38936 exported redirect_if_opac_suppressed Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189877 [details] [review] Bug 40680: Fix C4/Patroncards/Patroncard.pm Attempt to call undefined import method with arguments (StrWidth) via package C4::Creators::PDF (Perhaps you forgot to load the package?) at /kohadevbox/koha/C4/Patroncards/Patroncard.pm line 28. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189878 [details] [review] Bug 40680: Fix acqui scripts Attempt to call undefined import method with arguments (get_active) via package Koha::Acquisition::Currencies (Perhaps you forgot to load the package?) at acqui/addorder.pl line 133. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189879 [details] [review] Bug 40680: Fix admin/columns_settings.pl Attempt to call undefined import method with arguments (get_modules) via package C4::Utils::DataTables::TablesSettings (Perhaps you forgot to load the package?) at admin/columns_settings.pl line 8. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189880 [details] [review] Bug 40680: Fix admin/matching-rules.pl Attempt to call undefined import method with arguments (valid_normalization_routines) via package C4::Matcher (Perhaps you forgot to load the package?) at admin/matching-rules.pl line 27. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189881 [details] [review] Bug 40680: Fix TestBuilder perl -wc misc/devel/create_test_data.pl Attempt to call undefined import method with arguments ("schema") via package "Koha::Database" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 5. Attempt to call undefined import method with arguments ("_type") via package "Koha::Biblios" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 7. Attempt to call undefined import method with arguments ("_type") via package "Koha::Items" (Perhaps you forgot to load the package?) at /kohadevbox/koha/t/lib/TestBuilder.pm line 8. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189882 [details] [review] Bug 40680: Fix svc/virtualshelves/search Attempt to call undefined import method with arguments (search) via package C4::Utils::DataTables::VirtualShelves (Perhaps you forgot to load the package?) at svc/virtualshelves/search line 8. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189883 [details] [review] Bug 40680: Fix t/Labels* Attempt to call undefined import method with arguments (split_callnumber) via package C4::ClassSplitRoutine::LCC (Perhaps you forgot to load the package?) at t/Labels.t line 22. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189884 [details] [review] Bug 40680: Fix C4::Calendar imports Attempt to call undefined import method with arguments (new ...) via package C4::Calendar (Perhaps you forgot to load the package?) at t/db_dependent/Hold.t line 25. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 189885 [details] [review] Bug 40680: Fix t tests Amended patch @ t/db_dependent/Realtime_holdsqueue.t:26 @ use Test::MockModule; -use C4::SIP::ILS::Transaction::Checkin qw( do_checkin ); +use C4::SIP::ILS::Transaction::Checkin; Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> I spend a fair chunk of time testing and reviewing this today and feel it's ready to go. Passing QA, but happy for it to go either side of release to be honest. Thanks a lot Martin! |