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: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Julian Maurice
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks: 37245
  Show dependency treegraph
 
Reported: 2024-01-31 13:04 UTC by Marcel de Rooy
Modified: 2024-11-18 17:48 UTC (History)
7 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:
24.11.00,24.05.06
Circulation function:


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
Bug 35959: Fix C3 merge of 5 modules (1.15 KB, patch)
2024-05-20 06:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Old::Hold (1.62 KB, patch)
2024-05-20 06:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Biblio::Metadata::Extractor::MARC::* (1.25 KB, patch)
2024-05-20 06:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Account::Debit, Koha::Account::Credit (1.10 KB, patch)
2024-05-20 06:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Old::Hold (part 2) (726 bytes, patch)
2024-07-03 09:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of 5 modules (1.21 KB, patch)
2024-07-03 11:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Old::Hold (1.69 KB, patch)
2024-07-03 11:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Biblio::Metadata::Extractor::MARC::* (1.32 KB, patch)
2024-07-03 11:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Account::Debit, Koha::Account::Credit (1.17 KB, patch)
2024-07-03 11:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35959: Fix C3 merge of Koha::Old::Hold (part 2) (793 bytes, patch)
2024-07-03 11:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35959: Add missing use statements in tests (1.23 KB, patch)
2024-07-05 21:17 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 (tcohen) 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)
Comment 40 David Nind 2024-05-19 20:44:00 UTC
I'm unsure from the comments whether this is ready for sign off or not.

Can confirm that with the patches, 'syntax OK' is received for all the perl -c commands.
Comment 41 Julian Maurice 2024-05-20 06:13:22 UTC
(In reply to David Nind from comment #40)
> I'm unsure from the comments whether this is ready for sign off or not.
> 
> Can confirm that with the patches, 'syntax OK' is received for all the perl
> -c commands.

Then please sign off. QA will decide what to do with these patches
Comment 42 David Nind 2024-05-20 06:21:30 UTC
Created attachment 166926 [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'

Signed-off-by: David Nind <david@davidnind.com>
Comment 43 David Nind 2024-05-20 06:21:33 UTC
Created attachment 166927 [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'

Signed-off-by: David Nind <david@davidnind.com>
Comment 44 David Nind 2024-05-20 06:21:36 UTC
Created attachment 166928 [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'

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 David Nind 2024-05-20 06:21:39 UTC
Created attachment 166929 [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'

Signed-off-by: David Nind <david@davidnind.com>
Comment 46 Martin Renvoize (ashimema) 2024-07-03 07:00:40 UTC
Sorry, this one introduces a test failure:

Proving t/db_dependent/Koha/Old/Hold.t KO!
    # No tests run!
 
#   Failed test 'No tests run for subtest "anonymize() tests"'
#   at t/db_dependent/Koha/Old/Hold.t line 82.
Can't locate object method "_new_from_dbic" via package "Koha::Old::Holds" at /kohadevbox/koha/Koha/Patron.pm line 1587.
 # Looks like your test exited with 255 just after 1.
 t/db_dependent/Koha/Old/Hold.t .. 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 3/3 subtests 
 
Test Summary Report
-------------------
t/db_dependent/Koha/Old/Hold.t (Wstat: 65280 (exited 255) Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 3 tests but ran 1.
Files=1, Tests=1,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.75 cusr  0.08 csys =  0.85 CPU)
Result: FAIL

        * Proving t/db_dependent/Reserves.t KO!
No reserves HOLD_CANCELLATION letter transported by email at /kohadevbox/koha/C4/Letters.pm line 610.
     # Looks like you planned 4 tests but ran 3.

#   Failed test 'MoveReserve additional test'
#   at t/db_dependent/Reserves.t line 1127.
 Can't locate object method "_new_from_dbic" via package "Koha::Old::Holds" at /kohadevbox/koha/Koha/Patron.pm line 1587.
 # Looks like your test exited with 11 just after 64.
 t/db_dependent/Reserves.t .. 
Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 14/77 subtests
Comment 47 Julian Maurice 2024-07-03 09:02:36 UTC
Created attachment 168427 [details] [review]
Bug 35959: Fix C3 merge of Koha::Old::Hold (part 2)
Comment 48 Julian Maurice 2024-07-03 11:26:38 UTC
t/db_dependent/Reserves.t still fail for me but it's the same failures as on main branch, so it should be ok
Comment 49 Martin Renvoize (ashimema) 2024-07-03 11:49:34 UTC
Created attachment 168441 [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'

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize (ashimema) 2024-07-03 11:49:37 UTC
Created attachment 168442 [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'

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize (ashimema) 2024-07-03 11:49:41 UTC
Created attachment 168443 [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'

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize (ashimema) 2024-07-03 11:49:45 UTC
Created attachment 168444 [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'

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 53 Martin Renvoize (ashimema) 2024-07-03 11:49:50 UTC
Created attachment 168445 [details] [review]
Bug 35959: Fix C3 merge of Koha::Old::Hold (part 2)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize (ashimema) 2024-07-03 11:51:45 UTC
All working now for me, thanks Julian.

I'm going to PQA here...

But.. on the chat, you mentioned an xt/perl-check.t test I'm not seeing anywhere.. be great to prevent this re-occurring in the future with a test.
Comment 55 Julian Maurice 2024-07-03 12:00:09 UTC
(In reply to Martin Renvoize from comment #54)
> But.. on the chat, you mentioned an xt/perl-check.t test I'm not seeing
> anywhere.. be great to prevent this re-occurring in the future with a test.
Ah... Apparently I wrote that a while ago and forgot about it... -_-"
I will create a new bug report for that
Comment 56 Julian Maurice 2024-07-03 13:25:35 UTC
(In reply to Julian Maurice from comment #55)
> I will create a new bug report for that
=> bug 37245
Comment 57 Jonathan Druart 2024-07-04 11:14:06 UTC
If we are starting that, we should put and write down where our limits are. Otherwise people are going to do that everywhere...
Comment 58 Julian Maurice 2024-07-04 12:39:00 UTC
(In reply to Jonathan Druart from comment #57)
> If we are starting that, we should put and write down where our limits are.
> Otherwise people are going to do that everywhere...
Ok, let me try:

> PERLXX: Prefer "use" than "require"
> 
> "use" is the preferred method of importing Perl modules. You should always use "use" when possible.
> All "use" statements should appear at the top of the file.
> 
> "require" is allowed when "use" cannot be used or when it causes problems. Allowed uses of "require" include:
> - Loading modules whose name is known only at runtime (for instance, plugins)
> - Fixing circular dependency problems (aka "Inconsistent hierarchy during C3 merge")
> 
> If you think you need to use "require" for something that is not in the list above, please contact a QA team member first to confirm it's OK.
I would add to the list that "making a script run faster" or "making a script use less memory" can also be valid uses. But it really depends on how much we gain VS how much modifications are required, so I would let that to QA judgement.
Comment 59 Jonathan Druart 2024-07-04 14:45:21 UTC
(In reply to Julian Maurice from comment #58)
> (In reply to Jonathan Druart from comment #57)
> > If we are starting that, we should put and write down where our limits are.
> > Otherwise people are going to do that everywhere...
> Ok, let me try:
> 
> > PERLXX: Prefer "use" than "require"
> > 
> > "use" is the preferred method of importing Perl modules. You should always use "use" when possible.
> > All "use" statements should appear at the top of the file.
> > 
> > "require" is allowed when "use" cannot be used or when it causes problems. Allowed uses of "require" include:
> > - Loading modules whose name is known only at runtime (for instance, plugins)
> > - Fixing circular dependency problems (aka "Inconsistent hierarchy during C3 merge")
> > 
> > If you think you need to use "require" for something that is not in the list above, please contact a QA team member first to confirm it's OK.

Thanks, excellent! I didn't notice you actually got rid of all of them.

> I would add to the list that "making a script run faster" or "making a
> script use less memory" can also be valid uses. But it really depends on how
> much we gain VS how much modifications are required, so I would let that to
> QA judgement.

Yes, and this is when it's hard to decide.
For instance: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158003
This patch exists only because C4::Context is badly written...
Comment 60 Katrin Fischer 2024-07-05 12:46:49 UTC
Anything to learn from this that should be added to documentation?
Comment 61 Katrin Fischer 2024-07-05 13:50:26 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 62 Julian Maurice 2024-07-05 13:59:51 UTC
Should I write the new coding guideline in the wiki or should we vote ?
Comment 63 Jonathan Druart 2024-07-05 14:07:12 UTC
(In reply to Julian Maurice from comment #62)
> Should I write the new coding guideline in the wiki or should we vote ?

Now it's too late it's pushed :D

Just add it I'd say.
Comment 64 Katrin Fischer 2024-07-05 14:08:23 UTC
(In reply to Julian Maurice from comment #62)
> Should I write the new coding guideline in the wiki or should we vote ?

Please add it and add a note to the next Developer meeting agenda, so we advertise it.
Comment 66 Katrin Fischer 2024-07-05 14:28:38 UTC
Thanks, Julian!
Comment 67 Katrin Fischer 2024-07-05 15:08:32 UTC
I suspect this has to do with the test failures we see on Jenkins - please check!
Comment 68 Katrin Fischer 2024-07-05 15:08:51 UTC
 t_db_dependent_Reserves_CancelExpiredReserves_t.Test handling of cancellation reason if passed (2000)	0 ms	1
 t_db_dependent_Koha_Holds_t.cancel (2044)	1 ms	1
 t_db_dependent_Koha_Holds_t.Number of runned tests does not match plan. (2044)
Comment 69 Julian Maurice 2024-07-05 21:17:08 UTC
Created attachment 168566 [details] [review]
Bug 35959: Add missing use statements in tests
Comment 70 Julian Maurice 2024-07-05 21:17:46 UTC
This patch fix both tests
Comment 71 Katrin Fischer 2024-07-08 13:46:59 UTC
Thanks for the follow-up! Let's try this :)
Comment 72 Lucas Gass (lukeg) 2024-11-18 17:48:11 UTC
Backported to 24.05.x for upcoming 24.05.06