Description
Marcel de Rooy
2024-01-31 13:04:20 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: 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) 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. (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 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> 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> 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> 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> 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 Created attachment 168427 [details] [review] Bug 35959: Fix C3 merge of Koha::Old::Hold (part 2) t/db_dependent/Reserves.t still fail for me but it's the same failures as on main branch, so it should be ok 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> 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> 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> 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> 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> 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. (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 (In reply to Julian Maurice from comment #55) > I will create a new bug report for that => bug 37245 If we are starting that, we should put and write down where our limits are. Otherwise people are going to do that everywhere... (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. (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... Anything to learn from this that should be added to documentation? Pushed for 24.11! Well done everyone, thank you! Should I write the new coding guideline in the wiki or should we vote ? (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. (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. Done: https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL31:_Prefer_.22use.22_than_.22require.22 https://wiki.koha-community.org/wiki/Development_IRC_meeting_17_July_2024#Agenda Thanks, Julian! I suspect this has to do with the test failures we see on Jenkins - please check! 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) Created attachment 168566 [details] [review] Bug 35959: Add missing use statements in tests This patch fix both tests Thanks for the follow-up! Let's try this :) Backported to 24.05.x for upcoming 24.05.06 |