Running perl -cw on Koha/AuthorisedValue gives me on master and 22.11 the following: Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue': current merge results [ Koha::AuthorisedValue, ] merging failed on 'Koha::Object' at /usr/lib/x86_64-linux-gnu/perl-base/base.pm line 186. BEGIN failed--compilation aborted at Koha/AuthorisedValue.pm line 25. This needs attention. Probably related to object inheritance etc. use base qw(Koha::Object Koha::Object::Limit::Library); Do we really need them both here as parents?
commit bd01041ac6985656ca1bf1f2a6b9d565e98bd61d Author: Tomás Cohen Arazi <tomascohen@theke.io> Date: Mon Dec 12 14:17:44 2016 -0300 Bug 17755: Introduce Koha::Object::Limit::Library This patch introduces a new class for extending Koha::Object using multiple inheritance. It cannot be used standalone, it needs to be used in Koha::Object implementations like this:
commit 95ab03b73ab2a1f021be119dabfaf826b3192d01 Author: Tomás Cohen Arazi <tomascohen@theke.io> Date: Fri Jul 5 16:23:52 2019 -0300 Bug 23272: Make Koha::AuthorisedValue use Koha::Object::Limit::Library
See also bug 23272 comment5. The error was introduced at the introduction of the module already.
perl -cw Koha/Patron/Attribute/Type.pm Inconsistent hierarchy during C3 merge of class 'Koha::Patron::Attribute::Type': current merge results [ Koha::Patron::Attribute::Type, ] merging failed on 'Koha::Object' at /usr/lib/x86_64-linux-gnu/perl-base/base.pm line 186. BEGIN failed--compilation aborted at Koha/Patron/Attribute/Type.pm line 23. See bug 17755
Koha/Account/DebitType.pm:use base qw(Koha::Object Koha::Object::Limit::Library); Koha/AuthorisedValue.pm:use base qw(Koha::Object Koha::Object::Limit::Library); Koha/ItemType.pm:use base qw(Koha::Object Koha::Object::Limit::Library); Koha/Object/Limit/Library.pm:package Koha::Object::Limit::Library; Koha/Object/Limit/Library.pm:Koha::Object::Limit::Library - Generic library limit handling class Koha/Object/Limit/Library.pm: use base qw(Koha::Object Koha::Object::Limit::Library); Koha/Objects/Limit/Library.pm: use base qw(Koha::Objects Koha::Object::Limit::Library); Koha/Patron/Attribute/Type.pm:use base qw(Koha::Object Koha::Object::Limit::Library); Koha/Patron/Category.pm:use base qw(Koha::Object Koha::Object::Limit::Library); There are more modules suffering here..
Created attachment 161690 [details] [review] Bug 35959: Resolve Inconsistent hierarchy-warn from 4 modules Inspired by https://www.perlmonks.org/?node_id=1151073 Clearing @ISA did not work, but pushing the relations seems to. Test plan: Run qa tools (to see results of perl -cw). (Ignore warning when processing files before patches.) Run some tests that use those modules. Touch a few pages referencing those modules. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested: prove t/db_dependent/AuthorisedValues.t t/db_dependent/Koha.t prove t/db_dependent/AuthorisedValues.t t/db_dependent/Koha.t t/db_dependent/Koha/Object.t prove t/db_dependent/Koha/Patron.t t/db_dependent/Koha/Patron/Attribute.t t/db_dependent/Koha/Patron/Attribute/Types.t prove t/db_dependent/Koha/Objects.t t/db_dependent/Koha/Patron/Categories.t admin/debit_types.pl admin/authorised_values.pl admin/patron-attr-types.pl admin/categories.pl
Tomas, Jonathan: Any feedback ?
Treat as bugfix or rather enhancment?
Created attachment 161692 [details] [review] Bug 35959: Resolve Inconsistent hierarchy-warn from 4 modules Inspired by https://www.perlmonks.org/?node_id=1151073 Clearing @ISA did not work, but pushing the relations seems to. Test plan: Run qa tools (to see results of perl -cw). (Ignore warning when processing files before patches.) Run some tests that use those modules. Touch a few pages referencing those modules. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested: prove t/db_dependent/Accounts.t t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/AuthorisedValues.t t/db_dependent/Koha.t t/db_dependent/Koha/Object.t prove t/db_dependent/Koha/Patron.t t/db_dependent/Koha/Patron/Attribute.t t/db_dependent/Koha/Patron/Attribute/Types.t prove t/db_dependent/Koha/Objects.t t/db_dependent/Koha/Patron/Categories.t admin/debit_types.pl admin/authorised_values.pl admin/patron-attr-types.pl admin/categories.pl
This might even be better: # Bug 35959 - Avoid Inconsistent hierarchy warn by replacing use base line @Koha::AuthorisedValue::ISA = qw(Koha::Object Koha::Object::Limit::Library);
(In reply to Marcel de Rooy from comment #10) > This might even be better: > > # Bug 35959 - Avoid Inconsistent hierarchy warn by replacing use base line > @Koha::AuthorisedValue::ISA = qw(Koha::Object Koha::Object::Limit::Library); Seems to resolve qa tools before/after processing: Error happened when reloading the module Koha::AuthorisedValue between the two steps. Might not be a blocker for QA: Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue':
Created attachment 161693 [details] [review] Bug 35959: (follow-up) Do not push, but refill ISA Test plan: See previous patch. Run qa tools on patch set that touches one of the modules. (I used 28869 touching AuthorisedValue.pm.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 162410 [details] [review] Bug 35959: Resolve Inconsistent hierarchy-warn from 4 modules Inspired by https://www.perlmonks.org/?node_id=1151073 Clearing @ISA did not work, but pushing the relations seems to. Test plan: Run qa tools (to see results of perl -cw). (Ignore warning when processing files before patches.) Run some tests that use those modules. Touch a few pages referencing those modules. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested: prove t/db_dependent/Accounts.t t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/AuthorisedValues.t t/db_dependent/Koha.t t/db_dependent/Koha/Object.t prove t/db_dependent/Koha/Patron.t t/db_dependent/Koha/Patron/Attribute.t t/db_dependent/Koha/Patron/Attribute/Types.t prove t/db_dependent/Koha/Objects.t t/db_dependent/Koha/Patron/Categories.t admin/debit_types.pl admin/authorised_values.pl admin/patron-attr-types.pl admin/categories.pl Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 162411 [details] [review] Bug 35959: (follow-up) Do not push, but refill ISA Test plan: See previous patch. Run qa tools on patch set that touches one of the modules. (I used 28869 touching AuthorisedValue.pm.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I was able to do the test plan of the 1st patch. But for the second, bug 28869 conflicts on multiple files with the patches here. I made a test commit where in AuthorisedValue.pm I swapped "use Koha::Caches;" and "use Koha::Database;" And I was expecting that this commit alone would trip the qa script into outputting "Inconsistent hierarchy during C3 merge" And that applying the patches here would fix that. But no. So I don't understand enough what this is about to fix the test plan.
So in a sense, patch does not apply. It's in between that and fail QA because the 2nd test plan isn't really valid anymore. > And I was expecting that this commit alone would trip the qa script into outputting "Inconsistent hierarchy during C3 merge" Or something like "Constant subroutine Koha::AuthorisedValue::NUM_PATTERN redefined" because that's what 28869 alone do.
I didn't notice bug 28869 has this one as dependency. So I was applying these patches on top of 28869 + these patches. So of course it conflicts. So no conflicts when applied properly, back to NSO. 1st patch test plan with both patchsets applied: ok 2nd test plan: > Run qa tools on patch set that touches one of the modules. > (I used 28869 touching AuthorisedValue.pm.) I don't know what the test plan expects here, is that ok?: FAIL Koha/AuthorisedValue.pm FAIL valid Constant subroutine Koha::AuthorisedValue::NUM_PATTERN redefined Constant subroutine Koha::AuthorisedValue::NUM_PATTERN_JS redefined
Thx Victor for testing. This might well be dependent on the perl version of the Debian version (or other distro) you are using. I had these warnings on D11. I know that other devs saw the warn too with other Koha modules(?), but AuthorisedValue seems to trigger it too. You can ignore the constant redefine warns for now. This is about the hierarchy.
So is that all to sign off? Just to have no relevant complain by the QA tools? --- After rereading comment 0 , I ran `perl -cw Koha/AuthorisedValue.pm` without patches and I get ``` Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue': current merge results [ Koha::AuthorisedValue, ] merging failed on 'Koha::Object' at /usr/share/perl/5.28/base.pm line 186. BEGIN failed--compilation aborted at Koha/AuthorisedValue.pm line 25. ``` And with the patches I don't get that anymore. Now it feels like having tested something that show the patches do something. :D Was there a part of the test plan supposed to show that and I messed it up?
Created attachment 162481 [details] [review] Bug 35959: Resolve Inconsistent hierarchy-warn from 4 modules Inspired by https://www.perlmonks.org/?node_id=1151073 Clearing @ISA did not work, but pushing the relations seems to. Test plan: 1. Run `perl -cw Koha/AuthorisedValue.pm` 2. You should get "Inconsistent hierarchy during C3 merge" 3. Apply the patches 4. Run `perl -cw Koha/AuthorisedValue.pm` 5. No more "Inconsistent hierarchy during C3 merge" 6. Run qa tools (to see results of perl -cw). (Ignore warning when processing files before patches.) 7. Run some tests that use those modules. (see the following "Signoff-off-by section") 8. Touch a few pages referencing those modules. (see the following "Signoff-off-by section") 9. See next patch's test plan Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested: prove t/db_dependent/Accounts.t t/db_dependent/Koha/Account/DebitType.t prove t/db_dependent/AuthorisedValues.t t/db_dependent/Koha.t t/db_dependent/Koha/Object.t prove t/db_dependent/Koha/Patron.t t/db_dependent/Koha/Patron/Attribute.t t/db_dependent/Koha/Patron/Attribute/Types.t prove t/db_dependent/Koha/Objects.t t/db_dependent/Koha/Patron/Categories.t admin/debit_types.pl admin/authorised_values.pl admin/patron-attr-types.pl admin/categories.pl Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 162482 [details] [review] Bug 35959: (follow-up) Do not push, but refill ISA Test plan: - Run qa tools on patch set that touches one of the modules. (I used bug 28869 touching AuthorisedValue.pm. it depends on this bug, so just apply 28869 on main/master) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 162483 [details] [review] Bug 35959: (follow-up) Do not push, but refill ISA Test plan: - Run qa tools on patch set that touches one of the modules. (I used bug 28869 touching AuthorisedValue.pm. it depends on this bug, so just apply 28869 on main/master) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
I completed and tried to clarify the test plan, let me know if I got something wrong. So, look like it works :)
(In reply to Victor Grousset/tuxayo from comment #23) > I completed and tried to clarify the test plan, let me know if I got > something wrong. > > So, look like it works :) Great. Thanks. My test plan is always too short etc..
I suspect I wasn't the only one that needed some guesswork to test this. Because I'm going from oldest to newest bugs so I'm mostly doing stuff that has been likely already looked at and skipped. Might be also that refactoring and code cleanup isn't the most attractive stuff. Anyway, it's back on track again ^o^ cleanup++
I mean... This is clearly a Perl bug :-P It is a bad sign that it hasn't been reported+solved already upstream.
(In reply to Tomás Cohen Arazi from comment #26) > I mean... > > This is clearly a Perl bug :-P > > It is a bad sign that it hasn't been reported+solved already upstream. Life is hard :) Please QA this one.
What do we want here? Hide the warnings or fix the original cause? IMO we are only hidding warnings and I don't understand how it can be useful to hide them. If we want to hide them from the QA script it can be done there. Note: Inconsistent hierarchy during C3 merge of class 'Koha::Account::Credit'
(In reply to Jonathan Druart from comment #28) > What do we want here? Hide the warnings or fix the original cause? This more or less fixes a perl bug like Tomas stated. Read fix, not hide.
(In reply to Marcel de Rooy from comment #29) > This more or less fixes a perl bug like Tomas stated. Read fix, not hide. There's no evidence it's a Perl bug, and given the state of Koha (circular dependencies everywhere), I'm more inclined to think it's a Koha bug, and that the patches are essentially hiding the fact that these circular dependencies exist. The error about Koha::AuthorisedValue can also be fixed by moving 'use Koha::Libraries' to a 'require Koha::Libraries' inside the right subroutine in Koha/Object/Limit/Library.pm. I'd prefer this kind of solution instead of manipulating @ISA manually.
I just submitted bug 36432. It removes a circular dependency. It doesn't fix the problem with Koha::AuthorisedValue, but I think we should try to go this way instead of messing with the load order of modules
(In reply to Julian Maurice from comment #31) > I just submitted bug 36432. It removes a circular dependency. It doesn't fix > the problem with Koha::AuthorisedValue, but I think we should try to go this > way instead of messing with the load order of modules Fine with me. It was a nice exercise ;)
Reopening as this is not fixed yet (and the bug I opened was not intended to fix it) I will submit new patches
Created attachment 164468 [details] [review] Bug 35959: Fix C3 merge of 5 modules Test plan: Run the following commands: perl -c Koha/AuthorisedValue.pm perl -c Koha/Patron/Category.pm perl -c Koha/Patron/Attribute/Type.pm perl -c Koha/Account/DebitType.pm perl -c Koha/Account/CreditType.pm They should all print 'syntax OK'
Created attachment 164469 [details] [review] Bug 35959: Fix C3 merge of Koha::Old::Hold Test plan: Run `perl -c Koha/Old/Hold.pm` It should print 'syntax OK'
Created attachment 164470 [details] [review] Bug 35959: Fix C3 merge of Koha::Biblio::Metadata::Extractor::MARC::* Test plan: Run the following commands: perl -c Koha/Biblio/Metadata/Extractor/MARC/MARC21.pm perl -c Koha/Biblio/Metadata/Extractor/MARC/UNIMARC.pm They should all print 'syntax OK'
Created attachment 164471 [details] [review] Bug 35959: Fix C3 merge of Koha::Account::Debit, Koha::Account::Credit Test plan: Run the following commands perl -c Koha/Account/Debit.pm perl -c Koha/Account/Credit.pm They should all print 'syntax OK'
I don't think we should replace use with require "just" to get rid of a warning.
(In reply to Jonathan Druart from comment #38) > I don't think we should replace use with require "just" to get rid of a > warning. It's not the best solution, I agree. But it's the best I could do with a reasonable amount of time. And it's not a warning, the module fails the `perl -c` check which is really bad IMO. Also, what is so bad about require ? it loads a module only when needed, which is a good thing I think. (for instance ...::Extractor::MARC::UNIMARC never needs to be loaded on a MARC21 instance, yet it is loaded every time through the "Koha::Biblio -> Koha::Biblio::Extractor -> Koha::Biblio::Extractor::MARC" dependency)