Bug 41521 - WebService::ILS::OverDrive not passing pl_valid
Summary: WebService::ILS::OverDrive not passing pl_valid
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 40680
Blocks: 41077
  Show dependency treegraph
 
Reported: 2026-01-05 12:56 UTC by Jonathan Druart
Modified: 2026-02-24 00:34 UTC (History)
2 users (show)

See Also:
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:


Attachments
Bug 41521: Remove deps from cpanfile (643 bytes, patch)
2026-01-23 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Add WebService::ILS to lib/ (92.23 KB, patch)
2026-01-23 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Remove perl valid warnings (1.96 KB, patch)
2026-01-23 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Reject lib/ from tidy check (642 bytes, patch)
2026-01-23 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Remove deps from cpanfile (643 bytes, patch)
2026-01-23 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Exclude lib/WebService from pod xt tests (1.88 KB, patch)
2026-01-23 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Fix spelling (3.79 KB, patch)
2026-01-23 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Fix forbidden patterns (trailing spaces and http) (5.18 KB, patch)
2026-01-23 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Silent perlcritic warnings (2.70 KB, patch)
2026-01-23 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Add WebService::ILS to lib/ (92.23 KB, patch)
2026-01-23 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Remove perl valid warnings (1.96 KB, patch)
2026-01-23 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Reject lib/ from tidy check (642 bytes, patch)
2026-01-23 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 41521: Add WebService::ILS to lib/ (92.25 KB, patch)
2026-02-07 17:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 41521: Remove perl valid warnings (2.01 KB, patch)
2026-02-07 17:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 41521: Reject lib/ from tidy check (696 bytes, patch)
2026-02-07 17:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 41521: Remove deps from cpanfile (697 bytes, patch)
2026-02-07 17:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 41521: Exclude lib/WebService from pod xt tests (1.93 KB, patch)
2026-02-07 17:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 41521: Fix spelling (3.84 KB, patch)
2026-02-07 17:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 41521: Fix forbidden patterns (trailing spaces and http) (3.41 KB, patch)
2026-02-07 17:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 41521: Silent perlcritic warnings (2.74 KB, patch)
2026-02-07 17:28 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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/"?