Bug 41076

Summary: Perltidy config needs to be refined to not cause changes with perltidy 20250105
Product: Koha Reporter: Victor Grousset/tuxayo <victor>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: Pushed to main --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: david, jonathan.druart, lucas, m.de.rooy, martin.renvoize, mtj
Version: MainKeywords: rel_25_05_candidate, rel_25_11_candidate
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Crowdfunding committed: 0
Crowdfunding contact: Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
26.05.00
Version(s) released in:
Circulation function:
Bug Depends on:    
Bug Blocks: 41077    
Attachments: Bug 41076: Adjust our perl tidiness for v20250105
Bug 41076: Adjust our perl tidiness for v20250105
Bug 41076: [25.05.x] Adjust perl tidiness

Description Victor Grousset/tuxayo 2025-10-23 00:06:36 UTC
1. KOHA_IMAGE=main-bookworm ktd up
2. prove xt/perltidy.t
3. leave running for like a minute, it passes, don't bother with full run
4. KOHA_IMAGE=main-trixie ktd up
5. prove xt/perltidy.t
6. failures quickly pile up

D12:
perltidy --version
This is perltidy, v20230309
dpkg -l | grep tidy
20230309-1~koha3 (ours)

D13:
perltidy --version
This is perltidy, v20250105
dpkg -l | grep tidy
20250105-1

So in KTD, the D13 image doesn't ship our perltidy so it uses Debian's which is more recent. And might use some different default that aren't covered by our current config. I guess it would be better to complete the config so that both version of perltidy output the same code, right? Rather than making the D13 image ship an old version of perltidy which would postpone the root issue.
Comment 1 Victor Grousset/tuxayo 2025-10-23 00:09:34 UTC
Also, it makes the qa script fail if working on Debian 13
Comment 2 Jonathan Druart 2025-10-23 10:43:06 UTC
(In reply to Victor Grousset/tuxayo from comment #0)
> So in KTD, the D13 image doesn't ship our perltidy so it uses Debian's which
> is more recent. And might use some different default that aren't covered by
> our current config. I guess it would be better to complete the config so
> that both version of perltidy output the same code, right?

Yes, that is what needs to be done.
Comment 3 Jonathan Druart 2026-01-23 08:54:37 UTC
Actually there are not many changes, and they are actually bugfixes

https://perltidy.sourceforge.net/ChangeLog.html

I would suggest to make v20250105 our default versions for the different OSes.
Comment 4 Jonathan Druart 2026-01-23 08:58:15 UTC
Created attachment 191896 [details] [review]
Bug 41076: Adjust our perl tidiness for v20250105

Generated using
  `perl misc/devel/tidy.pl --perl`
Comment 5 Jonathan Druart 2026-01-23 08:58:22 UTC
It's actually a lot of changes to test files, but they are greatly improving the readability!
Comment 6 Jonathan Druart 2026-01-23 08:58:51 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2026-01-23 09:08:29 UTC
Which rules actually change?
Comment 8 Jonathan Druart 2026-01-23 09:40:35 UTC
(In reply to Marcel de Rooy from comment #7)
> Which rules actually change?
As said there are bug fixes.

I found this one:

https://perltidy.sourceforge.net/ChangeLog.html
2025 01 05

- Space around here doc delimiters follow spacing controls better. For
example, a space is now added before the closing paren here:

   OLD: (without the here doc):
   push( @script, <<'EOT');

   NEW:
   push( @script, <<'EOT' );

Also, any spaces between the '<<' and here target are removed (git #174):

   OLD:
   push( @script, <<  'EOT');

   NEW:
   push( @script, <<'EOT' );


The other changes made sense to me (and felt like bug fixes as well) so it seems better to include the changes than adding options to ignore them.
Comment 9 Martin Renvoize (ashimema) 2026-01-26 13:51:26 UTC
Created attachment 192028 [details] [review]
Bug 41076: Adjust our perl tidiness for v20250105

Generated using
  `perl misc/devel/tidy.pl --perl`

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 10 Martin Renvoize (ashimema) 2026-01-26 13:53:18 UTC
I@m happy with the changes here.. I would have liked to indent params in search calls, but perltidy can't do that consistently.. 

We could add nesting to our code to get around it, but that also feels a bit dirty for little reward.

Certainly need to wait for Mason to get the go ahead before pushing this one though.
Comment 11 Jonathan Druart 2026-02-02 09:26:36 UTC
Do not push if v20250105 is not the default everywhere! See with Mason.
Comment 12 Jonathan Druart 2026-02-02 09:27:19 UTC
I am now seeing this on D12:

$ apt-cache policy perltidy
perltidy:
  Installed: 20250105-1
  Candidate: 20250105-1
  Version table:
 *** 20250105-1 500
        500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages
        100 /var/lib/dpkg/status
     20220613-1 500
        500 http://deb.debian.org/debian bookworm/main amd64 Packages
Comment 13 Mason James 2026-02-03 07:35:49 UTC
(In reply to Jonathan Druart from comment #12)
> I am now seeing this on D12:
> 
> $ apt-cache policy perltidy
> perltidy:
>   Installed: 20250105-1
>   Candidate: 20250105-1
>   Version table:
>  *** 20250105-1 500
>         500 http://debian.koha-community.org/koha-staging dev/main amd64
> Packages
>         100 /var/lib/dpkg/status
>      20220613-1 500
>         500 http://deb.debian.org/debian bookworm/main amd64 Packages

hiya, i've added perltidy 20250105-1 to the koha-staging repo

i just used debian's package, and did not build/rename a koha version
Comment 14 Mason James 2026-02-05 01:12:52 UTC
i've addeed this to prod APT repo, for all active branches


$ apt policy perltidy
perltidy:
  Installed: 20250105-1~koha1
  Candidate: 20250105-1~koha1
  Version table:
 *** 20250105-1~koha1 500
        500 https://debian.koha-community.org/koha stable/main amd64 Packages
        500 https://debian.koha-community.org/koha stable/main armhf Packages
        100 /var/lib/dpkg/status
     20200110-1 500
        500 http://ftp.nz.debian.org/debian bullseye/main amd64 Packages
        500 http://ftp.nz.debian.org/debian bullseye/main armhf Packages
Comment 15 Martin Renvoize (ashimema) 2026-02-05 10:55:05 UTC
Checking KTD's, we now have:

D13: This is perltidy, v20250105

D12: This is perltidy, v20250105

D11: This is perltidy, v20250105

U24: Fails to start on my machine

U22: This is perltidy, v20250105

So.. all the KTD's now have the same modern perltidy.. which means we need these patches applied and any new code needs to by retidied on push to match the new rules if it now fails due to the new perltidy version.
Comment 16 Lucas Gass (lukeg) 2026-02-05 16:04:58 UTC
Pushed to main for 26.05.00.
Comment 17 Owen Leonard 2026-02-05 20:00:04 UTC
*** Bug 41735 has been marked as a duplicate of this bug. ***
Comment 18 Marcel de Rooy 2026-02-06 07:49:31 UTC
(In reply to Martin Renvoize (ashimema) from comment #15)
> Checking KTD's, we now have:
> 
> D13: This is perltidy, v20250105
> 
> D12: This is perltidy, v20250105
> 
> D11: This is perltidy, v20250105
> 
> U24: Fails to start on my machine
> 
> U22: This is perltidy, v20250105
> 
> So.. all the KTD's now have the same modern perltidy.. which means we need
> these patches applied and any new code needs to by retidied on push to match
> the new rules if it now fails due to the new perltidy version.

I am running on Debian 12 and note that not everyone runs KTD !
perltidy -v
This is perltidy, v20230309

Please help the non-KTD minority ;)
Comment 19 Marcel de Rooy 2026-02-06 07:59:03 UTC
(In reply to Marcel de Rooy from comment #18)
> (In reply to Martin Renvoize (ashimema) from comment #15)
> > Checking KTD's, we now have:
> > 
> > D13: This is perltidy, v20250105
> > 
> > D12: This is perltidy, v20250105
> > 
> > D11: This is perltidy, v20250105
> > 
> > U24: Fails to start on my machine
> > 
> > U22: This is perltidy, v20250105
> > 
> > So.. all the KTD's now have the same modern perltidy.. which means we need
> > these patches applied and any new code needs to by retidied on push to match
> > the new rules if it now fails due to the new perltidy version.
> 
> I am running on Debian 12 and note that not everyone runs KTD !
> perltidy -v
> This is perltidy, v20230309
> 
> Please help the non-KTD minority ;)

Note that D12 even has an older version. This one already came from debian.koha-community. So a rebuild should fix my container :)
Comment 20 Marcel de Rooy 2026-02-06 08:12:51 UTC
Just a side note but just intrigued by the first perltidy change in Auth.pm:
@@ -1236,7 +1236,7 @@ sub checkauth {
                             : Koha::Cash::Registers->search(
                             { branch => $branchcode, branch_default => 1 },
                             { rows   => 1 }
-                        )->single;
+                            )->single;

I would have preferred to indent the two lines after ->search here.. Now the search parameters in the ternary else branch are on the same level as the single applied on the whole ternary result.
Comment 21 Jonathan Druart 2026-02-09 09:15:54 UTC
(In reply to Marcel de Rooy from comment #20)
> Just a side note but just intrigued by the first perltidy change in Auth.pm:
> @@ -1236,7 +1236,7 @@ sub checkauth {
>                              : Koha::Cash::Registers->search(
>                              { branch => $branchcode, branch_default => 1 },
>                              { rows   => 1 }
> -                        )->single;
> +                            )->single;
> 
> I would have preferred to indent the two lines after ->search here.. Now the
> search parameters in the ternary else branch are on the same level as the
> single applied on the whole ternary result.

Martin asked the same on Mattermost: https://chat.koha-community.org/koha-community/pl/4j1pnibow7n9tdhcjddgzs1w4c

The whole statement (now):

1233                             my $register =
1234                                 $query->param('register_id')
1235                                 ? Koha::Cash::Registers->find( $query->param('register_id') )
1236                                 : Koha::Cash::Registers->search(
1237                                 { branch => $branchcode, branch_default => 1 },
1238                                 { rows   => 1 }
1239                                 )->single;

1239 was definitely wrong, and it's fixed now. However I do agree that 1237 and 1238 should have an additional indent level. Maybe it will be fixed in the next update? :)
Comment 22 Lucas Gass (lukeg) 2026-02-24 15:33:47 UTC
Created attachment 193768 [details] [review]
Bug 41076: [25.05.x] Adjust perl tidiness