Bug 35959 - Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue' (and a few other modules)
Summary: Inconsistent hierarchy during C3 merge of class 'Koha::AuthorisedValue' (and ...
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-01-31 13:04 UTC by Marcel de Rooy
Modified: 2024-04-17 00:06 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 35959: Resolve Inconsistent hierarchy-warn from 4 modules (3.73 KB, patch)
2024-01-31 14:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35959: Resolve Inconsistent hierarchy-warn from 4 modules (3.74 KB, patch)
2024-01-31 14:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35959: (follow-up) Do not push, but refill ISA (2.65 KB, patch)
2024-01-31 15:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35959: Resolve Inconsistent hierarchy-warn from 4 modules (3.75 KB, patch)
2024-02-25 21:15 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35959: (follow-up) Do not push, but refill ISA (2.61 KB, patch)
2024-02-25 21:15 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35959: Resolve Inconsistent hierarchy-warn from 4 modules (4.10 KB, patch)
2024-02-27 06:32 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35959: (follow-up) Do not push, but refill ISA (2.66 KB, patch)
2024-02-27 06:32 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35959: (follow-up) Do not push, but refill ISA (2.71 KB, patch)
2024-02-27 06:32 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of 5 modules (1.10 KB, patch)
2024-04-05 12:30 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Old::Hold (1.57 KB, patch)
2024-04-05 12:30 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Biblio::Metadata::Extractor::MARC::* (1.21 KB, patch)
2024-04-05 12:30 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Account::Debit, Koha::Account::Credit (1.06 KB, patch)
2024-04-05 12:30 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2024-01-31 13:04:20 UTC
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?
Comment 1 Marcel de Rooy 2024-01-31 13:05:39 UTC
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:
Comment 2 Marcel de Rooy 2024-01-31 13:11:09 UTC
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
Comment 3 Marcel de Rooy 2024-01-31 13:11:41 UTC
See also bug 23272 comment5.
The error was introduced at the introduction of the module already.
Comment 4 Marcel de Rooy 2024-01-31 13:21:17 UTC
 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
Comment 5 Marcel de Rooy 2024-01-31 13:22:48 UTC
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..
Comment 6 Marcel de Rooy 2024-01-31 14:49:21 UTC
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
Comment 7 Marcel de Rooy 2024-01-31 14:49:51 UTC
Tomas, Jonathan: Any feedback ?
Comment 8 Marcel de Rooy 2024-01-31 14:51:15 UTC
Treat as bugfix or rather enhancment?
Comment 9 Marcel de Rooy 2024-01-31 14:54:34 UTC
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
Comment 10 Marcel de Rooy 2024-01-31 15:04:35 UTC
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);
Comment 11 Marcel de Rooy 2024-01-31 15:07:54 UTC
(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':
Comment 12 Marcel de Rooy 2024-01-31 15:11:31 UTC
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>
Comment 13 Victor Grousset/tuxayo 2024-02-25 21:15:11 UTC
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>
Comment 14 Victor Grousset/tuxayo 2024-02-25 21:15:14 UTC
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>
Comment 15 Victor Grousset/tuxayo 2024-02-25 21:19:39 UTC
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.
Comment 16 Victor Grousset/tuxayo 2024-02-25 21:25:46 UTC
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.
Comment 17 Victor Grousset/tuxayo 2024-02-26 05:23:31 UTC
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
Comment 18 Marcel de Rooy 2024-02-26 07:59:48 UTC
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.
Comment 19 Victor Grousset/tuxayo 2024-02-27 06:22:20 UTC
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?
Comment 20 Victor Grousset/tuxayo 2024-02-27 06:32:01 UTC
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>
Comment 21 Victor Grousset/tuxayo 2024-02-27 06:32:04 UTC Comment hidden (obsolete)
Comment 22 Victor Grousset/tuxayo 2024-02-27 06:32:42 UTC
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>
Comment 23 Victor Grousset/tuxayo 2024-02-27 06:36:23 UTC
I completed and tried to clarify the test plan, let me know if I got something wrong.

So, look like it works :)
Comment 24 Marcel de Rooy 2024-02-27 12:23:06 UTC
(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..
Comment 25 Victor Grousset/tuxayo 2024-02-28 04:42:43 UTC
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++
Comment 26 Tomás Cohen Arazi 2024-03-11 18:40:57 UTC
I mean...

This is clearly a Perl bug :-P

It is a bad sign that it hasn't been reported+solved already upstream.
Comment 27 Marcel de Rooy 2024-03-12 11:59:41 UTC
(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.
Comment 28 Jonathan Druart 2024-03-12 12:53:14 UTC
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'
Comment 29 Marcel de Rooy 2024-03-12 13:05:53 UTC
(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.
Comment 30 Julian Maurice 2024-03-26 10:37:11 UTC
(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.
Comment 31 Julian Maurice 2024-03-26 15:49:00 UTC
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
Comment 32 Marcel de Rooy 2024-03-26 15:52:43 UTC
(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 ;)
Comment 33 Julian Maurice 2024-04-05 12:28:56 UTC
Reopening as this is not fixed yet (and the bug I opened was not intended to fix it)
I will submit new patches
Comment 34 Julian Maurice 2024-04-05 12:30:18 UTC
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'
Comment 35 Julian Maurice 2024-04-05 12:30:21 UTC
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'
Comment 36 Julian Maurice 2024-04-05 12:30:24 UTC
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'
Comment 37 Julian Maurice 2024-04-05 12:30:27 UTC
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'
Comment 38 Jonathan Druart 2024-04-05 12:43:39 UTC
I don't think we should replace use with require "just" to get rid of a warning.
Comment 39 Julian Maurice 2024-04-05 13:18:55 UTC
(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)