Bug 29771 - Get rid of CGI::param in list context warnings
Summary: Get rid of CGI::param in list context warnings
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 18320 21051 29758
Blocks: 25790
  Show dependency treegraph
 
Reported: 2021-12-23 19:07 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:23 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.04,21.05.14


Attachments
Bug 29771: Remove warning from acqui/finishreceive.pl (1.29 KB, patch)
2021-12-23 19:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29771: Remove trivial cases (4.64 KB, patch)
2021-12-23 20:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29771: Scalar context for split (4.05 KB, patch)
2021-12-23 20:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29771: Remove trivial cases (4.70 KB, patch)
2022-02-25 19:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29771: Scalar context for split (4.10 KB, patch)
2022-02-25 19:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29771: Remove trivial cases (4.76 KB, patch)
2022-03-01 15:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29771: Scalar context for split (4.17 KB, patch)
2022-03-01 15:08 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-12-23 19:07:54 UTC

    
Comment 1 Tomás Cohen Arazi 2021-12-23 19:12:28 UTC
$ git grep '\->param' | grep -v -e 'template' -e 'multi_param' | grep '@' | wc -l
54
Comment 2 Tomás Cohen Arazi 2021-12-23 19:19:25 UTC
Created attachment 128908 [details] [review]
Bug 29771: Remove warning from acqui/finishreceive.pl

This patch removes a warning that shows when receiving.

To test:
1. Do the acq workflow up to the receive step.
2. Once you choose the items and click on Finish
=> FAIL: There's a warning in the logs
3. Revert receipt
4. Apply this patch
5. Receive
=> SUCCESS: No more warnings
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-12-23 20:26:44 UTC
Created attachment 128909 [details] [review]
Bug 29771: Remove trivial cases

I squashed the patches because they are too trivial to have a test plan.
Or it is too much work to write the test plan for such trivial cases. I
leave the original commit messages just in case.

Generally, this are all cases in which CGI::param is being called in a
trivially identifiable _list context_. i.e. they are assigned to a
@variable.

I left one case out on purpose: admin/auth_subfields_structure.pl

Paul introduced this:

my @kohafield = ''.$input->param('kohafield');
and then:
my $kohafield = $kohafield[$i];

My intuition says it is forcing scalar context on the first assignment
so the list contains a single element and then inside the loop some
$kohafield assignments should lead to undef, and even warnings. I leave
it for a separate patch because it is not that easy testable and is a
sensible area.

Bug 29771: Remove warning from  acqui/finishreceive.pl

This patch removes a warning that shows when receiving.

To test:
1. Do the acq workflow up to the receive step.
2. Once you choose the items and click on Finish
=> FAIL: There's a warning in the logs
3. Revert receipt
4. Apply this patch
5. Receive
=> SUCCESS: No more warnings
6. Sign off :-D

Bug 29771: Remove warning from svc/members/add_to_list

To test:
1. Run:
   $ tail -f /var/log/koha/kohadev/*-error.log
2. Generate a patron list
3. Perform a patron search that gives you a few
4. Select some, and choose to add them to the list
=> FAIL: The logs show the infamous warn:
CGI::param called in list context from /kohadevbox/koha/svc/members/add_to_list
5. Apply this patch
6. Restart plack and repeat 4
=> SUCCESS: No warn!
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2021-12-23 20:26:48 UTC
Created attachment 128910 [details] [review]
Bug 29771: Scalar context for split

This patch tackles a very specific scenario. Calling split(...,
CGI::param) makes it be called in list context. The split docs say:

split /PATTERN/,EXPR,LIMIT

this means the first CGI param will be used as EXPR and the second one
as LIMIT, which is wrong anyway. So the fix is to just force scalar
context.

To test:
1. Not sure, just make sure nothing breaks when using the scripts in the
   browser.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Nick Clemens 2022-02-25 19:20:24 UTC
Created attachment 131129 [details] [review]
Bug 29771: Remove trivial cases

I squashed the patches because they are too trivial to have a test plan.
Or it is too much work to write the test plan for such trivial cases. I
leave the original commit messages just in case.

Generally, this are all cases in which CGI::param is being called in a
trivially identifiable _list context_. i.e. they are assigned to a
@variable.

I left one case out on purpose: admin/auth_subfields_structure.pl

Paul introduced this:

my @kohafield = ''.$input->param('kohafield');
and then:
my $kohafield = $kohafield[$i];

My intuition says it is forcing scalar context on the first assignment
so the list contains a single element and then inside the loop some
$kohafield assignments should lead to undef, and even warnings. I leave
it for a separate patch because it is not that easy testable and is a
sensible area.

Bug 29771: Remove warning from  acqui/finishreceive.pl

This patch removes a warning that shows when receiving.

To test:
1. Do the acq workflow up to the receive step.
2. Once you choose the items and click on Finish
=> FAIL: There's a warning in the logs
3. Revert receipt
4. Apply this patch
5. Receive
=> SUCCESS: No more warnings
6. Sign off :-D

Bug 29771: Remove warning from svc/members/add_to_list

To test:
1. Run:
   $ tail -f /var/log/koha/kohadev/*-error.log
2. Generate a patron list
3. Perform a patron search that gives you a few
4. Select some, and choose to add them to the list
=> FAIL: The logs show the infamous warn:
CGI::param called in list context from /kohadevbox/koha/svc/members/add_to_list
5. Apply this patch
6. Restart plack and repeat 4
=> SUCCESS: No warn!
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens 2022-02-25 19:20:27 UTC
Created attachment 131130 [details] [review]
Bug 29771: Scalar context for split

This patch tackles a very specific scenario. Calling split(...,
CGI::param) makes it be called in list context. The split docs say:

split /PATTERN/,EXPR,LIMIT

this means the first CGI param will be used as EXPR and the second one
as LIMIT, which is wrong anyway. So the fix is to just force scalar
context.

To test:
1. Not sure, just make sure nothing breaks when using the scripts in the
   browser.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Martin Renvoize 2022-03-01 15:08:14 UTC
Created attachment 131187 [details] [review]
Bug 29771: Remove trivial cases

I squashed the patches because they are too trivial to have a test plan.
Or it is too much work to write the test plan for such trivial cases. I
leave the original commit messages just in case.

Generally, this are all cases in which CGI::param is being called in a
trivially identifiable _list context_. i.e. they are assigned to a
@variable.

I left one case out on purpose: admin/auth_subfields_structure.pl

Paul introduced this:

my @kohafield = ''.$input->param('kohafield');
and then:
my $kohafield = $kohafield[$i];

My intuition says it is forcing scalar context on the first assignment
so the list contains a single element and then inside the loop some
$kohafield assignments should lead to undef, and even warnings. I leave
it for a separate patch because it is not that easy testable and is a
sensible area.

Bug 29771: Remove warning from  acqui/finishreceive.pl

This patch removes a warning that shows when receiving.

To test:
1. Do the acq workflow up to the receive step.
2. Once you choose the items and click on Finish
=> FAIL: There's a warning in the logs
3. Revert receipt
4. Apply this patch
5. Receive
=> SUCCESS: No more warnings
6. Sign off :-D

Bug 29771: Remove warning from svc/members/add_to_list

To test:
1. Run:
   $ tail -f /var/log/koha/kohadev/*-error.log
2. Generate a patron list
3. Perform a patron search that gives you a few
4. Select some, and choose to add them to the list
=> FAIL: The logs show the infamous warn:
CGI::param called in list context from /kohadevbox/koha/svc/members/add_to_list
5. Apply this patch
6. Restart plack and repeat 4
=> SUCCESS: No warn!
7. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-03-01 15:08:18 UTC
Created attachment 131188 [details] [review]
Bug 29771: Scalar context for split

This patch tackles a very specific scenario. Calling split(...,
CGI::param) makes it be called in list context. The split docs say:

split /PATTERN/,EXPR,LIMIT

this means the first CGI param will be used as EXPR and the second one
as LIMIT, which is wrong anyway. So the fix is to just force scalar
context.

To test:
1. Not sure, just make sure nothing breaks when using the scripts in the
   browser.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-03-01 15:09:50 UTC
Great call!

This fixes all obvious cases and many non-obvious ones too..

No regressions found, lets get this pushed and deal with any further occurrences as we spot them.

It would be nice to have a QA script test catch new introductions of such warnings.. but that can be handled outside of this bug.

Passing QA
Comment 10 Fridolin Somers 2022-03-04 01:54:59 UTC
Dependency with Bug 17239 ? isn't it the opposite relation ?
Comment 11 Tomás Cohen Arazi 2022-03-04 02:18:33 UTC
(In reply to Fridolin Somers from comment #10)
> Dependency with Bug 17239 ? isn't it the opposite relation ?

I think it is wrong. Probably a duplicate instead
Comment 12 Fridolin Somers 2022-03-04 06:02:22 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> (In reply to Fridolin Somers from comment #10)
> > Dependency with Bug 17239 ? isn't it the opposite relation ?
> 
> I think it is wrong. Probably a duplicate instead

OK I move to See Also
Comment 13 Fridolin Somers 2022-03-09 09:07:12 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 14 Kyle M Hall 2022-03-11 11:42:59 UTC
Pushed to 21.11.x for 21.11.04
Comment 15 Andrew Fuerste-Henry 2022-04-25 19:53:42 UTC
Pushed to 21.05.x for 21.05.14
Comment 16 Victor Grousset/tuxayo 2022-04-25 20:51:21 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.