Cleanup Perl warnings in 'make check' and logs

[Jason Boyer]

Eg master as of 2020-09-15

Over time I've collected examples of log noise from perl warnings and after running make check manually to check the perl modules there are several more. I'm going to try to put together a branch that calms those down (and shrink some logs!)

Sample warnings from make check:
Possible precedence issue with control flow operator at ...lib/OpenILS/Application/Booking.pm line 1286, <DATA> line 1.

Odd number of elements in anonymous hash at ...lib/OpenILS/Application/Search/Biblio.pm line 1905, <DATA> line 1.

"my" variable $results masks earlier declaration in same scope at ...lib/OpenILS/WWW/EGCatLoader/Course.pm line 305, <DATA> line 1.
CGI::param called in list context from ...lib/OpenILS/WWW/EGCatLoader/Search.pm line 16, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 412, <DATA> line 1.

And various logs hither and yon:
(I think I've seen the hold targeter send some of these, not sure which version)
CALL: open-ils.cstore open-ils.cstore.direct.action.circulation.search.atomic {"target_copy":[1234],"checkin_time":null,"duration":{">":"2 months"}},{"order_by":"due_date","limit":1}
"order_by" object in a query is not a JSON_HASH or JSON_ARRAY;no ORDER BY generated

(caused by things like ...search_class_here(...)[0] when no class_here is returned)
open-ils.cstore.direct.actor.copy_alert_suppress.search.atomic: returned 0 result(s)
open-ils.circ.checkin: substr outside of string at .../OpenILS/Application/AppUtils.pm line 2277.

"my" variable $circ_ids masks earlier declaration in same scope at ...OpenILS/WWW/EGCatLoader/Account.pm line 1936.

[Jason Boyer (jboyer)]

Here's a branch that addresses all of the make check issues (though the logger is still annoyed that there's no config - perhaps it could somehow be set to warn...) and a small selection of things recently found in osrfwarn.log: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jboyer/lp1895660_bring_the_noise_wait_dont / working/user/jboyer/lp1895660_bring_the_noise_wait_dont

It builds ok and nothing is obviously broken but I'd invite as much poking as folks can spare, especially around EGCatLoader/Search.pm because it has the largest number of changes. I want to avoid the warnings *and* the situation(s) being warned about...

I hesitate to open this door too widely but if you see some useless noise in your logs (actual noise; perl uninitialized variable stuff, not "This database is broken!") and can tell me how to trigger it in this bug I'll see what I can do about making it go away.

[Jason Boyer (jboyer)]

Also take note of the first commit: this is broken up by file for simpler review, I would hope there aren't 9 or more individual file commits if / when these go in.

[Jason Boyer (jboyer)]

Here are some basic notes on how to test these changes from the client:
Testing affected interfaces:

Booking: place some booking reservations and capture some of them. Open user account, make sure things look as expected in Other/Pickup Reservations, Manage Reservations, etc.

Search/Biblio.pm: do some searches. There is basically a 0% chance there are any issues with these changes.

Course.pm: set up some courses and reserve some titles, then search for them.

EGCatLoader/Search.pm: perform various types of searches comparing them to other concerto-dataset servers. Be sure to also use multiple search terms with and/or also.

HoldTargeter.pm: set up hold driven recalls as explained in http://docs.evergreen-ils.org/reorg/3.2/staff_client_admin/_hold_driven_recalls.html ,
back-date checkout all items on a bib so there are none available and the checkout dates are spread around so that some items fall within the recall threshold and some don't,
then when the hold targeter runs it should always prefer the "oldest" circulation to recall, and never one that's within the recall duration.

Application/AppUtils.pm: This change primarily affects Item Alerts. Circulate items with no alerts, some with alerts, and some with alerts that are suppressed.

Search/Authority.pm: Add or create authority records, search by heading.

Application/SuperCat.pm: https://(server)/opac/extras/unapi?id=tag::U2@bre/2{holdings_xml,acp}&format=marcxml should return results with no errors on a server with and without this change.

WWW/Proxy.pm: sign in to the reporter following a link in an email or by constructing a URL from the template/report/schedule ids.

[Jason Boyer (jboyer)]

Force pushed an update to take care of a couple "string" == "string" warnings. (should be "string" eq "string" and number == number)

[Jane Sandberg (sandbergja)]

Thanks so much for taking on this cleanup project, Jason! I poked at your branch for a while, and couldn't see any issues.

Signoff here: user/sandbergja/lp1895660_bring_the_noise_wait_dont

[Galen Charlton (gmc)]

Pushed all the way down to rel_3_6. Thanks, Jason and Jane!