Created attachment 153786 [details] [review] Bug 34339: (bug 33556 follow-up) Easy ERM-related changes I left the (easy) ERM cases out because it throws this when running the QA script: Processing files before patches |========================>| 51 / 51 (100.00%) An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Titles::Local, ] merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112. and didn't want this to pollute the rest of the changes. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
The error can easily be recreated redoing what does the QA script my @files = qw( Koha/REST/V1/ERM/Agreements.pm Koha/REST/V1/ERM/EHoldings/Titles/Local.pm Koha/REST/V1/ERM/EHoldings/Packages.pm Koha/REST/V1/ERM/Licenses.pm Koha/REST/V1/ERM/EHoldings/Titles/EBSCO.pm Koha/REST/V1/ERM/EHoldings/Packages/Local.pm ); for my $file (@files) { next if $file !~ m|\.pm$|; my $module = $file; delete $INC{$module}; $module =~ s|/|::|g; $module =~ s|\.pm$||; eval "require $module"; $module->import; } This appears before and after the patch. Should not be blocker.
I have opened https://gitlab.com/koha-community/qa-test-tools/-/issues/72
Created attachment 153796 [details] [review] Bug 34339: Avoid relying on $c->validation - ERM
Test plan: All EBSCO related actions must be tested in the eRM module.
From 33556: (In reply to Tomás Cohen Arazi from comment #21) > (In reply to Marcel de Rooy from comment #20) > > Please clarify > > I can't, and won't. It is an underlying problem with the modules design, > which the author(s) should address. My feeling is it is related to this: > > use Mojo::Base 'Mojolicious::Controller'; > > use Koha::ERM::EHoldings::Titles; Just comparing Biblios with ERM/Agreements here: === REST/V1/Biblios use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; use Koha::Biblios; use Koha::DateUtils; use Koha::Ratings; use Koha::RecordProcessor; use C4::Biblio qw( DelBiblio AddBiblio ModBiblio ); use C4::Search qw( FindDuplicate ); use C4::Barcodes::ValueBuilder; use C4::Context; use Koha::Items; use List::MoreUtils qw( any ); use MARC::Record::MiJ; use Try::Tiny qw( catch try ); === REST/V1/ERM/Agreements use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; use Koha::ERM::Agreements; use Scalar::Util qw( blessed ); use Try::Tiny qw( catch try ); I do not really see a difference here. Can anyone help me why ERM/Agreements is doing something different?
To provide a bit more context, the "Inconsistent hierarchy during C3 merge of class" error refers to method resolution order / C3 algorithm and might be caused by e.g. 'use parent' that adds a class twice in @ISA. I did not see parent here, but base. So should be fine? But looking at Mojo/Base, I do see some mangling of @ISA in there. At perlmonks someone suggested to clear @ISA at some point. Not sure how to proceed..
(In reply to Marcel de Rooy from comment #7) > To provide a bit more context, the "Inconsistent hierarchy during C3 merge > of class" error refers to method resolution order / C3 algorithm and might > be caused by e.g. 'use parent' that adds a class twice in @ISA. > I did not see parent here, but base. So should be fine? > But looking at Mojo/Base, I do see some mangling of @ISA in there. > At perlmonks someone suggested to clear @ISA at some point. > Not sure how to proceed.. It's not failing on Koha/REST/V1/ERM/Agreements.pm but the packages or titles packages. The error appears with only Koha/REST/V1/ERM/EHoldings/Packages.pm Koha/REST/V1/ERM/EHoldings/Packages/Local.pm Koha/REST/V1/ERM/EHoldings/Packages.pm use Mojo::Base 'Mojolicious::Controller'; use Koha::REST::V1::ERM::EHoldings::Packages::Local; use Koha::REST::V1::ERM::EHoldings::Packages::EBSCO; Koha/REST/V1/ERM/EHoldings/Packages/Local.pm use Mojo::Base 'Mojolicious::Controller';
But really, this is for another bug, it's not related to this patch.
Created attachment 155340 [details] [review] Bug 34339: (bug 33556 follow-up) Easy ERM-related changes I left the (easy) ERM cases out because it throws this when running the QA script: Processing files before patches |========================>| 51 / 51 (100.00%) An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Titles::Local, ] merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112. and didn't want this to pollute the rest of the changes. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 155341 [details] [review] Bug 34339: Avoid relying on $c->validation - ERM Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 155351 [details] [review] Bug 34339: (bug 33556 follow-up) Easy ERM-related changes I left the (easy) ERM cases out because it throws this when running the QA script: Processing files before patches |========================>| 51 / 51 (100.00%) An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Titles::Local, ] merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112. and didn't want this to pollute the rest of the changes. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155352 [details] [review] Bug 34339: Avoid relying on $c->validation - ERM Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155353 [details] [review] Bug 34339: (QA follow-up) Replace new by search in Licenses Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
WARN Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 25, now: 27) WARN Koha/REST/V1/ERM/EHoldings/Resources/EBSCO.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 54, now: 56) WARN Koha/REST/V1/ERM/EHoldings/Titles/EBSCO.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 21, now: 24) => Just warnings. No reason to block pushing imo. Corrected Koha/REST/V1/ERM/Licenses.pm - my $license = $c->objects->find( Koha::ERM::Licenses->search, $license_id ); + my $license = $c->objects->find( Koha::ERM::Licenses->new, $c->param('license_id') ); prove t/db_dependent/api/v1/erm* t/db_dependent/api/v1/erm_agreements.t ........... ok t/db_dependent/api/v1/erm_documents.t ............ ok t/db_dependent/api/v1/erm_eholdings_packages.t ... ok t/db_dependent/api/v1/erm_eholdings_resources.t .. ok t/db_dependent/api/v1/erm_eholdings_titles.t ..... ok t/db_dependent/api/v1/erm_licenses.t ............. ok t/db_dependent/api/v1/erm_users.t ................ ok All tests successful. Files=7, Tests=24, 78 wallclock secs ( 0.15 usr 0.05 sys + 68.57 cusr 5.75 csys = 74.52 CPU) Result: PASS
Any further idea/knowledge about: Processing files before patches Error happened when reloading the module Koha::REST::V1::ERM::EHoldings::Packages::EBSCO between the two steps. Might not be a blocker for QA: Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Packages::EBSCO': current merge results [ Koha::REST::V1::ERM::EHoldings::Packages::EBSCO, ] merging failed on 'Mojolicious::Controller' at /app/qatools/koha-qa.pl line 114. Error happened when reloading the module Koha::REST::V1::ERM::EHoldings::Resources::EBSCO between the two steps. Might not be a blocker for QA: Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Resources::EBSCO': current merge results [ Koha::REST::V1::ERM::EHoldings::Resources::EBSCO, ] merging failed on 'Mojolicious::Controller' at /app/qatools/koha-qa.pl line 114. Error happened when reloading the module Koha::REST::V1::ERM::EHoldings::Packages::Local between the two steps. Might not be a blocker for QA: Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Packages::Local': current merge results [ Koha::REST::V1::ERM::EHoldings::Packages::Local, ] merging failed on 'Mojolicious::Controller' at /app/qatools/koha-qa.pl line 114.
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.04
Dependency missing for 22.11.x - not backporting Nice work everyone!