Bug 30781

Summary: Use of uninitialized value $val in substitution iterator at /usr/share/koha/lib/C4/Letters.pm line 665.
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: Command-line UtilitiesAssignee: Fridolin Somers <fridolin.somers>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: trivial    
Priority: P5 - low CC: arthur.suzuki, fridolin.somers, jonathan.druart+koha, lucas, m.de.rooy, robin, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
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
Bug Depends on:    
Bug Blocks: 25790    
Attachments: Bug 30781: Add unit test for substitute with undefined value
Bug 30781: Fix warning in GetPreparedLetter
Bug 30781: Add unit test for substitute with undefined value
Bug 30781: Fix warning in GetPreparedLetter
Bug 30781: Add unit test for substitute with undefined value
Bug 30781: Fix warning in GetPreparedLetter
Bug 30781: Fix warning in GetPreparedLetter

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