Bug 24367

Summary: With strict enabled, Search.t is too verbose
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: 1joynelson, m.de.rooy, martin.renvoize, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 24217    
Bug Blocks:    
Attachments: Bug 24367: Remove warning from Search.t
Bug 24367: Remove warning from Search.t
Bug 24367: DO NOT PUSH - update MARC records for tests
Bug 24367: Resolve warn Argument available is not numeric in delete
Bug 24367: Resolve warn Negative repeat count does nothing
Bug 24367: Resolve warn Use of uninitialized value within [arrays]
Bug 24367: Resolve other uninitialized warns in Search.pm
Bug 24367: May fix another warning

Description Jonathan Druart 2020-01-07 15:53:39 UTC
There are tons of warnings displayed when running Search.t
Comment 1 Jonathan Druart 2020-01-07 15:55:47 UTC
Created attachment 96911 [details] [review]
Bug 24367: Remove warning from Search.t

Not done yet.
Comment 2 Jonathan Druart 2020-01-09 14:54:03 UTC
Created attachment 97096 [details] [review]
Bug 24367: Remove warning from Search.t

Not done yet.
Comment 3 Jonathan Druart 2020-01-09 14:57:06 UTC
Created attachment 97097 [details] [review]
Bug 24367: DO NOT PUSH - update MARC records for tests

The idea was to set the itemtype to a valid value (BK instead of BOOK)
The exported_records was generated like:
 * add 942$c=MX when no 942$c exist
 * replace 942$c=BOOK with 942$c=BK

But it did not remove remaining warnings
Comment 4 Jonathan Druart 2020-01-09 14:57:45 UTC
I need help for this one.
Comment 5 Marcel de Rooy 2020-01-10 08:17:29 UTC
Created attachment 97157 [details] [review]
Bug 24367: Resolve warn Argument available is not numeric in delete

Argument "available" isn't numeric in delete at /usr/share/koha/C4/Search.pm line 1480.
The construction delete array[string] simply does not work.
Comment 6 Marcel de Rooy 2020-01-10 08:17:34 UTC
Created attachment 97158 [details] [review]
Bug 24367: Resolve warn Negative repeat count does nothing

Negative repeat count does nothing at /usr/share/koha/Koha/QueryParser/Driver/PQF/query_plan.pm line 53.
Negative repeat count does nothing at /usr/share/koha/Koha/QueryParser/Driver/PQF/query_plan/node.pm line 102.

I am not sure if this statement actually does what the author intended,
but adding the test on node_count/atom_count removes the warn.
Repeating this change on query_plan.pm line 62.
Comment 7 Marcel de Rooy 2020-01-10 08:18:23 UTC
Had 42 warnings, now 28 left. Still working.
Comment 8 Marcel de Rooy 2020-01-10 08:30:59 UTC
Created attachment 97159 [details] [review]
Bug 24367: Resolve warn Use of uninitialized value within [arrays]

Use of uninitialized value within @operators in string eq at /usr/share/koha/C4/Search.pm line 1362.
Use of uninitialized value within @indexes in pattern match (m//) at /usr/share/koha/C4/Search.pm line 1367.

Trivial tests added.
Comment 9 Marcel de Rooy 2020-01-10 08:54:39 UTC
Created attachment 97163 [details] [review]
Bug 24367: Resolve other uninitialized warns in Search.pm

Use of uninitialized value in hash element at /usr/share/koha/C4/Search.pm line 2074.
Use of uninitialized value in hash element at /usr/share/koha/C4/Search.pm line 2137.
Use of uninitialized value in concatenation (.) or string at /usr/share/koha/C4/Search.pm line 2189.
Use of uninitialized value in hash element at /usr/share/koha/C4/Search.pm line 2213.
Use of uninitialized value in hash element at /usr/share/koha/C4/Search.pm line 2215.
Use of uninitialized value in hash element at /usr/share/koha/C4/Search.pm line 2225.
Use of uninitialized value in hash element at /usr/share/koha/C4/Search.pm line 2226.
Comment 10 Marcel de Rooy 2020-01-10 08:56:20 UTC
The last path silences them all but contains a few arguable ones. Should we silence ones where an itemtype should be filled?
Comment 11 Jonathan Druart 2020-01-10 09:27:18 UTC
(In reply to Marcel de Rooy from comment #10)
> The last path silences them all but contains a few arguable ones. Should we
> silence ones where an itemtype should be filled?

Even with the patch from comment 3?
Comment 12 Marcel de Rooy 2020-01-10 09:29:53 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Marcel de Rooy from comment #10)
> > The last path silences them all but contains a few arguable ones. Should we
> > silence ones where an itemtype should be filled?
> 
> Even with the patch from comment 3?

Yes on top of the first. I tested a bit with/without the second.
Comment 13 Jonathan Druart 2020-01-10 12:50:07 UTC
This is ready for testing.

Test plan:
Confirm that Search.t does not output warnings
Eyeball the patch and try to catch possible regressions
Comment 14 Martin Renvoize 2020-02-13 16:27:20 UTC
I still see one final warning: 

Use of uninitialized value in concatenation (.) or string at /kohadevbox/koha/C4/Search.pm line 1818.
Comment 15 Jonathan Druart 2020-02-17 10:31:30 UTC
(In reply to Martin Renvoize from comment #14)
> I still see one final warning: 
> 
> Use of uninitialized value in concatenation (.) or string at
> /kohadevbox/koha/C4/Search.pm line 1818.

I don't have this one, how did you get it?

kohadev-koha@b2404290bde5:/kohadevbox/koha$ prove t/db_dependent/Search.t
t/db_dependent/Search.t .. ok 
All tests successful.
Files=1, Tests=2, 12 wallclock secs ( 0.04 usr  0.01 sys +  7.26 cusr  1.60 csys =  8.91 CPU)
Result: PASS
Comment 16 Martin Renvoize 2020-02-17 14:59:56 UTC
Interesting.. I got it by just running that one test in koha-testing-docker on stretch.
Comment 17 Jonathan Druart 2020-02-17 15:59:45 UTC
Created attachment 99113 [details] [review]
Bug 24367: May fix another warning

May fix the following warning:
Use of uninitialized value in concatenation (.) or string at
/kohadevbox/koha/C4/Search.pm line 1818.
Comment 18 Jonathan Druart 2020-02-17 16:00:47 UTC
(In reply to Jonathan Druart from comment #17)
> Created attachment 99113 [details] [review] [review]
> Bug 24367: May fix another warning
> 
> May fix the following warning:
> Use of uninitialized value in concatenation (.) or string at
> /kohadevbox/koha/C4/Search.pm line 1818.

Martin, this patch may fix the warning you got. But I am not sure we push it if you are the only one to have it.
We should confirm with someone else.
I tried with ES and Zebra, and both passed without warning without this patch applied.
Comment 19 Martin Renvoize 2020-02-17 21:40:44 UTC
Lets push without that final patch.. hopefully it was just a blip on my system.
Comment 20 Martin Renvoize 2020-02-19 11:34:41 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Joy Nelson 2020-03-12 23:59:22 UTC
not backported due to missing dependencies