Bug 14185 - Noisy warns in opac-readingrecord.pl
Summary: Noisy warns in opac-readingrecord.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-12 05:14 UTC by Aleisha Amohia
Modified: 2016-12-05 21:23 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14185: Undefined $limit causes warn in opac/opac-readingrecord.pl (948 bytes, patch)
2015-05-12 05:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14185: Undefined $limit causes warn in opac/opac-readingrecord.pl (1.43 KB, patch)
2015-05-13 00:16 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
[PASSED QA] Bug 14185: Undefined $limit causes warn in opac/opac-readingrecord.pl (1.52 KB, patch)
2015-05-22 12:33 UTC, Kyle M Hall
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-05-12 05:14:59 UTC
If $limit is left undefined it causes a warn in error log
Comment 1 Aleisha Amohia 2015-05-12 05:20:00 UTC Comment hidden (obsolete)
Comment 2 Indranil Das Gupta 2015-05-13 00:16:02 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2015-05-13 04:44:07 UTC
Comment on attachment 39111 [details] [review]
Bug 14185: Undefined $limit causes warn in opac/opac-readingrecord.pl

Review of attachment 39111 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-readingrecord.pl
@@ +73,4 @@
>  
>  
>  my $limit = $query->param('limit');
> +$limit //= '';

This works, but why not add "// ''" to line 75?
Comment 4 Aleisha Amohia 2015-05-13 04:56:49 UTC
(In reply to M. Tompsett from comment #3)
> Comment on attachment 39111 [details] [review] [review]
> Bug 14185: Undefined $limit causes warn in opac/opac-readingrecord.pl
> 
> Review of attachment 39111 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: opac/opac-readingrecord.pl
> @@ +73,4 @@
> >  
> >  
> >  my $limit = $query->param('limit');
> > +$limit //= '';
> 
> This works, but why not add "// ''" to line 75?

I'm still very new to perl and had seen this warn solved this way before so just did it again. Of course if you think your way is cleaner then do that!
Comment 5 M. Tompsett 2015-05-13 17:48:59 UTC
(In reply to Aleisha Amohia from comment #4)
> I'm still very new to perl and had seen this warn solved this way before so
> just did it again.

Ah, okay. :) I didn't know. When I started perl, I was unaware of //. The difference between || and // is an important distinction to know. :)

undef || 'blah'; -- This is 'blah'
0     || 'blah'; -- This is 'blah'
''    || 'blah'; -- This is 'blah'
undef // 'blah'; -- This is 'blah'
0     // 'blah'; -- This is 0
''    // 'blah'; -- This is ''

As you can see // depends on whether the value is actually defined, not just its truth value. So, you actually chose the better operator inadvertently. :)
Comment 6 Kyle M Hall 2015-05-22 12:33:11 UTC
Created attachment 39403 [details] [review]
[PASSED QA] Bug 14185: Undefined $limit causes warn in opac/opac-readingrecord.pl

This patch sets $limit to be an empty string.

Test plan
=========

1/ login into the opac using your user account credentials
2/ in a terminal, run a `tail -f ` on your instance's opac-error.log
3/ go back to the opac, click on 'your reading history' tab to go to
   opac-readingrecord.pl
4/ notice the warning - "opac-readingrecord.pl: Use of uninitialized
   value $limit" appear in the `tail`ed opac-error.log
5/ apply the patch
6/ reload the page (opac-readingrecord.pl)
7/ page works but the warning in step #4 is no longer logged
8/ run qa test (i.e. koha-qa.pl -c 1 -v 2), there should be no error

Remarks: Testing result match expected test plan output. The QA tests
         pass with "OK" for the commit.

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Tomás Cohen Arazi 2015-05-26 14:06:11 UTC
Patch pushed to master.

Thanks Aleisha!
Comment 8 Chris Cormack 2015-05-27 08:03:07 UTC
Pushed to 3.20.x will be in 3.20.1
Comment 9 Liz Rea 2015-06-22 22:35:56 UTC
Patch pushed to 3.18.x will be in 3.18.08
Comment 10 Mason James 2015-07-25 09:34:10 UTC
Pushed to 3.16.x, will be in 3.16.13