Bug 41521

Summary: WebService::ILS::OverDrive not passing pl_valid
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: Passed QA --- QA Contact: David Cook <dcook>
Severity: enhancement    
Priority: P5 - low CC: dcook, victor
Version: unspecified   
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 40680    
Bug Blocks: 41077    
Attachments: Bug 41521: Remove deps from cpanfile
Bug 41521: Add WebService::ILS to lib/
Bug 41521: Remove perl valid warnings
Bug 41521: Reject lib/ from tidy check
Bug 41521: Remove deps from cpanfile
Bug 41521: Exclude lib/WebService from pod xt tests
Bug 41521: Fix spelling
Bug 41521: Fix forbidden patterns (trailing spaces and http)
Bug 41521: Silent perlcritic warnings
Bug 41521: Add WebService::ILS to lib/
Bug 41521: Remove perl valid warnings
Bug 41521: Reject lib/ from tidy check
Bug 41521: Add WebService::ILS to lib/
Bug 41521: Remove perl valid warnings
Bug 41521: Reject lib/ from tidy check
Bug 41521: Remove deps from cpanfile
Bug 41521: Exclude lib/WebService from pod xt tests
Bug 41521: Fix spelling
Bug 41521: Fix forbidden patterns (trailing spaces and http)
Bug 41521: Silent perlcritic warnings
Bug 41521: Add WebService::ILS to lib/
Bug 41521: Remove perl valid warnings
Bug 41521: Reject lib/ from tidy check
Bug 41521: Remove deps from cpanfile
Bug 41521: Exclude lib/WebService from pod xt tests
Bug 41521: Fix spelling
Bug 41521: Fix forbidden patterns (trailing spaces and http)
Bug 41521: Silent perlcritic warnings

Description Jonathan Druart 2026-01-05 12:56:53 UTC
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
Comment 1 Jonathan Druart 2026-01-05 12:58:59 UTC
Do we embed WebService::ILS::OverDrive in Koha (under lib)?

At least for a quick solution, and maybe improve the situation later.
Comment 2 David Cook 2026-01-05 22:07:53 UTC
(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.
Comment 3 Victor Grousset/tuxayo 2026-01-19 15:45:26 UTC
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
Comment 4 David Cook 2026-01-19 22:17:02 UTC
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.
Comment 5 Jonathan Druart 2026-01-23 09:06:42 UTC
Created attachment 191897 [details] [review]
Bug 41521: Remove deps from cpanfile

Patch from commit 2a15a6b
Comment 6 Jonathan Druart 2026-01-23 09:38:23 UTC
Created attachment 191898 [details] [review]
Bug 41521: Add WebService::ILS to lib/

Patch from commit a34697d
Comment 7 Jonathan Druart 2026-01-23 09:38:24 UTC
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.
Comment 8 Jonathan Druart 2026-01-23 09:38:25 UTC
Created attachment 191900 [details] [review]
Bug 41521: Reject lib/ from tidy check

Patch from commit 0978bb7
Comment 9 Jonathan Druart 2026-01-23 09:38:27 UTC
Created attachment 191901 [details] [review]
Bug 41521: Remove deps from cpanfile

Patch from commit 2a15a6b
Comment 10 Jonathan Druart 2026-01-23 09:38:28 UTC
Created attachment 191902 [details] [review]
Bug 41521: Exclude lib/WebService from pod xt tests

Patch from commit f10f0b8
Comment 11 Jonathan Druart 2026-01-23 09:38:29 UTC
Created attachment 191903 [details] [review]
Bug 41521: Fix spelling

Patch from commit 77e0a35
Comment 12 Jonathan Druart 2026-01-23 09:38:31 UTC
Created attachment 191904 [details] [review]
Bug 41521: Fix forbidden patterns (trailing spaces and http)

Patch from commit 19d32ef
Comment 13 Jonathan Druart 2026-01-23 09:38:32 UTC
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
Comment 14 Jonathan Druart 2026-01-23 09:38:45 UTC
The qa script is still raising errors on POD but they are going to be ignored by the tests and CI should not complain.
Comment 15 Jonathan Druart 2026-01-23 11:05:25 UTC
Created attachment 191917 [details] [review]
Bug 41521: Add WebService::ILS to lib/

Patch from commit a34697d
Comment 16 Jonathan Druart 2026-01-23 11:05:26 UTC
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.
Comment 17 Jonathan Druart 2026-01-23 11:05:27 UTC
Created attachment 191919 [details] [review]
Bug 41521: Reject lib/ from tidy check

Patch from commit 0978bb7
Comment 18 Victor Grousset/tuxayo 2026-02-07 17:28:30 UTC
Created attachment 192669 [details] [review]
Bug 41521: Add WebService::ILS to lib/

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Victor Grousset/tuxayo 2026-02-07 17:28:32 UTC
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>
Comment 20 Victor Grousset/tuxayo 2026-02-07 17:28:34 UTC
Created attachment 192671 [details] [review]
Bug 41521: Reject lib/ from tidy check

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 21 Victor Grousset/tuxayo 2026-02-07 17:28:36 UTC
Created attachment 192672 [details] [review]
Bug 41521: Remove deps from cpanfile

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 22 Victor Grousset/tuxayo 2026-02-07 17:28:38 UTC
Created attachment 192673 [details] [review]
Bug 41521: Exclude lib/WebService from pod xt tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 23 Victor Grousset/tuxayo 2026-02-07 17:28:40 UTC
Created attachment 192674 [details] [review]
Bug 41521: Fix spelling

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 24 Victor Grousset/tuxayo 2026-02-07 17:28:42 UTC
Created attachment 192675 [details] [review]
Bug 41521: Fix forbidden patterns (trailing spaces and http)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 25 Victor Grousset/tuxayo 2026-02-07 17:28:44 UTC
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>
Comment 26 Victor Grousset/tuxayo 2026-02-07 17:28:54 UTC
It works! :)
Comment 27 Victor Grousset/tuxayo 2026-02-18 04:44:38 UTC
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 28 David Cook 2026-02-20 05:45:13 UTC
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?
Comment 29 Jonathan Druart 2026-02-20 08:23:48 UTC
(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?
Comment 30 David Cook 2026-02-24 00:34:39 UTC
(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/"?
Comment 31 David Cook 2026-02-25 00:29:57 UTC
Anyway... I'm just splitting hairs a bit I think so let's not worry about my comment there. Like Jonathan said we can always change that later if need be.

"prove xt/pl_valid.t" takes a long time to run... I ran it to reproduce the original warning messages but to prove it after applying the patch I just did the following:

perl -cw Koha/ExternalContent/OverDrive.pm
perl -cw opac/external/overdrive/auth.pl
perl -cw opac/svc/overdrive

--

I'm not sure I understand the changes to Koha/Devel/Files.pm but I'm not too worried about that. Maybe it just means that we don't fail the "qa" tools for fails?

Otherwise... changes look fairly straightforward. The warnings go away after the patch. 

I trust Jonathan and Victor but I figure I'll do a quick comparison of the modules anyway using "diff -r /usr/share/perl5/WebService/ lib/WebService/"

Changes are minimal and logical, and they eliminate the problem. 

Looks solid.
Comment 32 David Cook 2026-02-25 00:30:36 UTC
Created attachment 193819 [details] [review]
Bug 41521: Add WebService::ILS to lib/

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 33 David Cook 2026-02-25 00:30:39 UTC
Created attachment 193820 [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>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 34 David Cook 2026-02-25 00:30:42 UTC
Created attachment 193821 [details] [review]
Bug 41521: Reject lib/ from tidy check

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 35 David Cook 2026-02-25 00:30:45 UTC
Created attachment 193822 [details] [review]
Bug 41521: Remove deps from cpanfile

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 36 David Cook 2026-02-25 00:30:49 UTC
Created attachment 193823 [details] [review]
Bug 41521: Exclude lib/WebService from pod xt tests

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 37 David Cook 2026-02-25 00:30:52 UTC
Created attachment 193824 [details] [review]
Bug 41521: Fix spelling

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 38 David Cook 2026-02-25 00:30:55 UTC
Created attachment 193825 [details] [review]
Bug 41521: Fix forbidden patterns (trailing spaces and http)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 39 David Cook 2026-02-25 00:30:58 UTC
Created attachment 193826 [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>
Signed-off-by: David Cook <dcook@prosentient.com.au>