Bug 30781 - Use of uninitialized value $val in substitution iterator at /usr/share/koha/lib/C4/Letters.pm line 665.
Summary: Use of uninitialized value $val in substitution iterator at /usr/share/koha/l...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low trivial (vote)
Assignee: Fridolin Somers
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 25790
  Show dependency treegraph
 
Reported: 2022-05-17 07:37 UTC by Katrin Fischer
Modified: 2023-06-08 22:26 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.01, 21.11.07, 21.05.16


Attachments
Bug 30781: Add unit test for substitute with undefined value (2.02 KB, patch)
2022-05-19 03:53 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30781: Fix warning in GetPreparedLetter (831 bytes, patch)
2022-05-19 03:53 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30781: Add unit test for substitute with undefined value (2.17 KB, patch)
2022-05-19 06:38 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30781: Fix warning in GetPreparedLetter (831 bytes, patch)
2022-05-19 06:38 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30781: Add unit test for substitute with undefined value (2.21 KB, patch)
2022-05-19 20:10 UTC, David Nind
Details | Diff | Splinter Review
Bug 30781: Fix warning in GetPreparedLetter (880 bytes, patch)
2022-05-19 20:10 UTC, David Nind
Details | Diff | Splinter Review
Bug 30781: Fix warning in GetPreparedLetter (938 bytes, patch)
2022-05-20 06:36 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2022-05-17 07:37:48 UTC
In our installation 20.11.17 it's these lines:

    if (%$substitute) {
        while ( my ($token, $val) = each %$substitute ) {
            if ( $token eq 'items.content' ) {
                $val =~ s|\n|<br/>|g if $letter->{is_html};
            }

-->            $letter->{title} =~ s/<<$token>>/$val/g;
            $letter->{content} =~ s/<<$token>>/$val/g;
       }
    }

Itemscontent is used as:

koha-foreach --chdir --enabled --email /usr/share/koha/bin/cronjobs/overdue_notices.pl -t -itemscontent date_due,title,subtitle,part_number,part_name,author,itemcallnumber,barcode

koha-foreach --chdir --enabled --email /usr/share/koha/bin/cronjobs/advance_notices.pl -c --itemscontent date_due,title,subtitle,part_number,part_name,author,itemcallnumber,barcode


This creates quite a bit of noise from the cronjobs, so it would be nice to get a hint on how to fix this or a patch.

In master the code in question is unchanged.
Comment 1 Jonathan Druart 2022-05-17 12:03:03 UTC
That could be fixed with the following change.

diff --git a/C4/Letters.pm b/C4/Letters.pm
index 9ed1de0e44f..8ab0bb727e0 100644
--- a/C4/Letters.pm
+++ b/C4/Letters.pm
@@ -599,6 +599,7 @@ sub GetPreparedLetter {
 
     if (%$substitute) {
         while ( my ($token, $val) = each %$substitute ) {
+            $val //= q{};
             if ( $token eq 'items.content' ) {
                 $val =~ s|\n|<br/>|g if $letter->{is_html};
             }
Comment 2 Fridolin Somers 2022-05-19 03:53:25 UTC
Created attachment 135177 [details] [review]
Bug 30781: Add unit test for substitute with undefined value

Add unit test that generates warn.
This may be usefull as no regression test.

Run prove t/db_dependent/Letters.t
You see warn :
t/db_dependent/Letters.t .. 1/84 Use of uninitialized value $val in substitution iterator at /kohadevbox/koha/C4/Letters.pm line 607.
Comment 3 Fridolin Somers 2022-05-19 03:53:48 UTC
Created attachment 135178 [details] [review]
Bug 30781: Fix warning in GetPreparedLetter

Test by running :
  prove t/db_dependent/Letters.t
Check you dont see warning :
  Use of uninitialized value $val in substitution iterator
Comment 4 Jonathan Druart 2022-05-19 04:45:18 UTC
To test that a subroutine call does not produce a warning you should use warning_is.
Comment 5 Fridolin Somers 2022-05-19 06:27:06 UTC
(In reply to Jonathan Druart from comment #4)
> To test that a subroutine call does not produce a warning you should use
> warning_is.

Ah OK.
I keep the test on the build of letter content and add a new for the warning.
Comment 6 Fridolin Somers 2022-05-19 06:38:06 UTC
Created attachment 135192 [details] [review]
Bug 30781: Add unit test for substitute with undefined value

Add unit test that generates warn.
This may be usefull as no regression test.

Run prove t/db_dependent/Letters.t
You see warn :
t/db_dependent/Letters.t .. 1/84 Use of uninitialized value $val in substitution iterator at /kohadevbox/koha/C4/Letters.pm line 607.
Comment 7 Fridolin Somers 2022-05-19 06:38:24 UTC
Created attachment 135193 [details] [review]
Bug 30781: Fix warning in GetPreparedLetter

Test by running :
  prove t/db_dependent/Letters.t
Check you dont see warning :
  Use of uninitialized value $val in substitution iterator
Comment 8 David Nind 2022-05-19 20:10:01 UTC
Created attachment 135219 [details] [review]
Bug 30781: Add unit test for substitute with undefined value

Add unit test that generates warn.
This may be usefull as no regression test.

Run prove t/db_dependent/Letters.t
You see warn :
t/db_dependent/Letters.t .. 1/84 Use of uninitialized value $val in substitution iterator at /kohadevbox/koha/C4/Letters.pm line 607.

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2022-05-19 20:10:06 UTC
Created attachment 135220 [details] [review]
Bug 30781: Fix warning in GetPreparedLetter

Test by running :
  prove t/db_dependent/Letters.t
Check you dont see warning :
  Use of uninitialized value $val in substitution iterator

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Katrin Fischer 2022-05-20 06:36:31 UTC
Created attachment 135222 [details] [review]
Bug 30781: Fix warning in GetPreparedLetter

Test by running :
  prove t/db_dependent/Letters.t
Check you dont see warning :
  Use of uninitialized value $val in substitution iterator

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Comment 11 Marcel de Rooy 2022-05-20 06:40:11 UTC
QAing. Not for this release
Comment 12 Marcel de Rooy 2022-05-20 06:40:27 UTC
(In reply to Marcel de Rooy from comment #11)
> QAing. Not for this release

Too late
Comment 13 Marcel de Rooy 2022-05-20 06:43:01 UTC
I wanted to add this remark:

We could have a theoretical debate if you should allow the substitution if you pass an undefined value. What we do now, is formalize what already happened but silence the warning. But we could also choose to skip this substitution since you should not pass undefined substitution parameters.
Comment 14 Tomás Cohen Arazi 2022-06-02 13:23:41 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 15 Lucas Gass 2022-06-10 16:36:29 UTC
Pushed to 22.05.x for 22.05.01
Comment 16 Arthur Suzuki 2022-06-21 13:34:40 UTC
thx, pushed to 21.11.x for 21.11.07
Comment 17 Victor Grousset/tuxayo 2022-06-25 23:57:13 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document, marking resolved.
Comment 18 Katrin Fischer 2022-06-26 09:51:06 UTC
I've ported this to 20.11 as it creates a lot of noise, might be nice in 21.05 as well.
Comment 19 Victor Grousset/tuxayo 2022-06-26 18:09:35 UTC
Backported: Pushed to 21.05.x branch for 21.05.16