Bug 14641 - Warns in subscription-add.pl
Summary: Warns in subscription-add.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-04 02:31 UTC by Aleisha Amohia
Modified: 2017-06-14 22:11 UTC (History)
6 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:


Attachments
Bug 14641: Warns in subscription-add.pl (1.03 KB, patch)
2015-08-04 02:34 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14641: Warns in subscription-add.pl (978 bytes, patch)
2015-12-13 22:06 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14641: Warns in subscription-add.pl (870 bytes, patch)
2016-01-11 21:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14641: [SIGNED-OFF] Warns in subscription-add.pl (995 bytes, patch)
2016-01-12 19:14 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 14641: [SIGNED-OFF] Warns in subscription-add.pl (1.04 KB, patch)
2016-01-13 11:53 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2015-08-04 02:31:03 UTC
subscription-add.pl: Use of uninitialized value $selected_lettercode in string eq at /home/vagrant/kohaclone/serials/subscription-add.pl line 231
subscription-add.pl: Use of uninitialized value $selected_lettercode in string eq at /home/vagrant/kohaclone/serials/subscription-add.pl line 231

To reproduce, go to Serials and click New Subscription.
Comment 1 Aleisha Amohia 2015-08-04 02:34:37 UTC Comment hidden (obsolete)
Comment 2 Joonas Kylmälä 2015-08-13 07:05:57 UTC
The warning is triggered when get_letter_loop() is called without an argument. We should maybe check if @_ is true, and if it's not then use $_ or "" as the value?
Comment 3 Jonathan Druart 2015-08-20 10:47:57 UTC
The brackets should be kept, for consistency at least.

I haven't tested, but this patch is certainly wrong.
Moreover, tests should be provided.
Comment 4 Aleisha Amohia 2015-12-13 22:06:29 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-12-28 16:36:01 UTC
Aleisha, with this patch the parameter sent to the subroutine is not taken into account anymore.
Comment 6 Aleisha Amohia 2016-01-10 22:42:21 UTC
(In reply to Joonas Kylmälä from comment #2)
> The warning is triggered when get_letter_loop() is called without an
> argument. We should maybe check if @_ is true, and if it's not then use $_
> or "" as the value?

I'm pretty new to perl so don't really know how I would write this. Do you have any suggestions so that we can give this bug a patch?
Comment 7 Jonathan Druart 2016-01-11 09:36:32 UTC
The verbose way to do it is:

    my ($selected_lettercode) = @_;
    if ( not defined $selected_lettercode ) {
        $selected_lettercode = '';
    }

A better way, IMO is to use `unless` instead of `if not`

    my ($selected_lettercode) = @_;
    unless ( defined $selected_lettercode ) {
        $selected_lettercode = '';
    }

A much more better way is to do the same but more readable (because less verbose):

    my ($selected_lettercode) = @_;
    $selected_lettercode //= '';

`//=` means `assign what is at the right if what is at the left is not defined`

`||=` would means `assign what is at the right if what is at the left is not defined or is 0 or is an empty string`
Comment 8 Aleisha Amohia 2016-01-11 21:17:01 UTC Comment hidden (obsolete)
Comment 9 Magnus Enger 2016-01-12 19:14:12 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-01-13 11:53:50 UTC
Created attachment 46580 [details] [review]
Bug 14641: [SIGNED-OFF] Warns in subscription-add.pl

Removes warn using Jonathan's suggestion!

To test:
1) Reproduce warns by clicking New Subscription
2) Apply patch
3) Click New Subscription again
4) Confirm warns are gone

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Patch removes the warn that come when clicking "New subscription"

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Brendan Gallagher 2016-01-27 04:32:17 UTC
Pushed to Master - Should be in the May 2016 release. Thanks!
Comment 12 Julian Maurice 2016-01-29 14:52:05 UTC
Patch pushed to 3.22.x, will be in 3.22.3
Comment 13 Frédéric Demians 2016-02-12 06:42:58 UTC
This patch has been pushed to 3.20.x, will be in 3.20.9.