From bug 40680 comment 23 Those patches fix most of the issues. Remaining ones: not ok 455 - Koha/ExternalContent/OverDrive.pm has syntax warnings. # Failed test 'Koha/ExternalContent/OverDrive.pm has syntax warnings.' # at xt/pl_valid.t line 63. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 365. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 573. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 643. not ok 2266 - opac/external/overdrive/auth.pl has syntax warnings. # Failed test 'opac/external/overdrive/auth.pl has syntax warnings.' # at xt/pl_valid.t line 63. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 365. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 573. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 643. not ok 2356 - opac/svc/overdrive has syntax warnings. # Failed test 'opac/svc/overdrive has syntax warnings.' # at xt/pl_valid.t line 63. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 365. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 573. # Possible precedence issue with control flow operator (die) at /usr/share/perl5/WebService/ILS/OverDrive/Patron.pm line 643. Test plan: Startup ktd using trixie's image export DB_IMAGE=mariadb:latest; export KOHA_IMAGE=main-trixie; ktd pull && ktd down && ktd --selenium --es7 up Run prove xt/pl_valid.t
Do we embed WebService::ILS::OverDrive in Koha (under lib)? At least for a quick solution, and maybe improve the situation later.
(In reply to Jonathan Druart from comment #1) > Do we embed WebService::ILS::OverDrive in Koha (under lib)? > > At least for a quick solution, and maybe improve the situation later. Yeah, I think that's probably going to be the only viable option for now.
There are already 2 other libs forked there. So yep, why not a third. Subclassing WebService/ILS/OverDrive/Patron.pm and overriding the relevant methods would introduce another way of managing such forking without an obvious benefit vs using /lib
I suppose one thing to keep in mind is that we might have to update other code than just those few lines so that it passes our tests. I bumped into that when subclassing Template::Stash::XS. But that's all right.
Created attachment 191897 [details] [review] Bug 41521: Remove deps from cpanfile Patch from commit 2a15a6b
Created attachment 191898 [details] [review] Bug 41521: Add WebService::ILS to lib/ Patch from commit a34697d
Created attachment 191899 [details] [review] Bug 41521: Remove perl valid warnings % perl -wc Koha/ExternalContent/OverDrive.pm Possible precedence issue with control flow operator (die) at /kohadevbox/koha/lib/WebService/ILS/OverDrive/Patron.pm line 365. Possible precedence issue with control flow operator (die) at /kohadevbox/koha/lib/WebService/ILS/OverDrive/Patron.pm line 573. Possible precedence issue with control flow operator (die) at /kohadevbox/koha/lib/WebService/ILS/OverDrive/Patron.pm line 643.
Created attachment 191900 [details] [review] Bug 41521: Reject lib/ from tidy check Patch from commit 0978bb7
Created attachment 191901 [details] [review] Bug 41521: Remove deps from cpanfile Patch from commit 2a15a6b
Created attachment 191902 [details] [review] Bug 41521: Exclude lib/WebService from pod xt tests Patch from commit f10f0b8
Created attachment 191903 [details] [review] Bug 41521: Fix spelling Patch from commit 77e0a35
Created attachment 191904 [details] [review] Bug 41521: Fix forbidden patterns (trailing spaces and http) Patch from commit 19d32ef
Created attachment 191905 [details] [review] Bug 41521: Silent perlcritic warnings Stricture disabled at line 67, column 5. See page 429 of PBP. (Severity: 5) We should deal with that later instead of ignoring the warnings
The qa script is still raising errors on POD but they are going to be ignored by the tests and CI should not complain.
Created attachment 191917 [details] [review] Bug 41521: Add WebService::ILS to lib/ Patch from commit a34697d
Created attachment 191918 [details] [review] Bug 41521: Remove perl valid warnings % perl -wc Koha/ExternalContent/OverDrive.pm Possible precedence issue with control flow operator (die) at /kohadevbox/koha/lib/WebService/ILS/OverDrive/Patron.pm line 365. Possible precedence issue with control flow operator (die) at /kohadevbox/koha/lib/WebService/ILS/OverDrive/Patron.pm line 573. Possible precedence issue with control flow operator (die) at /kohadevbox/koha/lib/WebService/ILS/OverDrive/Patron.pm line 643.
Created attachment 191919 [details] [review] Bug 41521: Reject lib/ from tidy check Patch from commit 0978bb7
Created attachment 192669 [details] [review] Bug 41521: Add WebService::ILS to lib/ Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 192670 [details] [review] Bug 41521: Remove perl valid warnings % perl -wc Koha/ExternalContent/OverDrive.pm Possible precedence issue with control flow operator (die) at /kohadevbox/koha/lib/WebService/ILS/OverDrive/Patron.pm line 365. Possible precedence issue with control flow operator (die) at /kohadevbox/koha/lib/WebService/ILS/OverDrive/Patron.pm line 573. Possible precedence issue with control flow operator (die) at /kohadevbox/koha/lib/WebService/ILS/OverDrive/Patron.pm line 643. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 192671 [details] [review] Bug 41521: Reject lib/ from tidy check Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 192672 [details] [review] Bug 41521: Remove deps from cpanfile Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 192673 [details] [review] Bug 41521: Exclude lib/WebService from pod xt tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 192674 [details] [review] Bug 41521: Fix spelling Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 192675 [details] [review] Bug 41521: Fix forbidden patterns (trailing spaces and http) Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 192676 [details] [review] Bug 41521: Silent perlcritic warnings Stricture disabled at line 67, column 5. See page 429 of PBP. (Severity: 5) We should deal with that later instead of ignoring the warnings Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
It works! :)
QA note: no accidental changes done to the lib And the changes look good. With another signoff I could pass QA. (compared with: # checkout commit 1 # then in KTD: mkdir /kohadevbox/koha/tmp cp -r /usr/share/perl5/WebService /kohadevbox/koha/tmp/ # host: meld tmp/WebService lib/WebService )
Comment on attachment 192671 [details] [review] Bug 41521: Reject lib/ from tidy check Review of attachment 192671 [details] [review]: ----------------------------------------------------------------- ::: Koha/Devel/Files.pm @@ +35,4 @@ > qw( > Koha/Schema/Result > Koha/Schema.pm > + lib/ At some point, I was hoping that we'd start moving our modules into this directory though?
(In reply to David Cook from comment #28) > Comment on attachment 192671 [details] [review] [review] > Bug 41521: Reject lib/ from tidy check > > Review of attachment 192671 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Devel/Files.pm > @@ +35,4 @@ > > qw( > > Koha/Schema/Result > > Koha/Schema.pm > > + lib/ > > At some point, I was hoping that we'd start moving our modules into this > directory though? Yes, "at some point" :D We will adjust when we will be there I'd say. We will certainly need to move those to lib/External or lib/ThirdParty. But this is not a new pattern. What do you suggest exactly?
(In reply to Jonathan Druart from comment #29) > (In reply to David Cook from comment #28) > > Comment on attachment 192671 [details] [review] [review] [review] > > Bug 41521: Reject lib/ from tidy check > > > > Review of attachment 192671 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: Koha/Devel/Files.pm > > @@ +35,4 @@ > > > qw( > > > Koha/Schema/Result > > > Koha/Schema.pm > > > + lib/ > > > > At some point, I was hoping that we'd start moving our modules into this > > directory though? > > Yes, "at some point" :D Haha fair. > We will adjust when we will be there I'd say. We will certainly need to move > those to lib/External or lib/ThirdParty. But this is not a new pattern. I'm not sure what you mean here? > What do you suggest exactly? I just mean rather than adding "lib/" to Koha/Devel/Files.pm, perhaps it would be better just to add "lib/WebService/ILS/OverDrive/"?