Bug 30196 - Odd number of elements warning caused by dt_from_string
Summary: Odd number of elements warning caused by dt_from_string
Status: RESOLVED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Blou
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25790
  Show dependency treegraph
 
Reported: 2022-02-28 17:11 UTC by Blou
Modified: 2023-06-17 15:54 UTC (History)
4 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 30196: Odd number of elements warning caused by dt_from_string (1.13 KB, patch)
2022-02-28 17:18 UTC, Blou
Details | Diff | Splinter Review
Bug 30196: Odd number of elements warning caused by dt_from_string (1.56 KB, patch)
2022-06-27 17:33 UTC, Shi Yao Wang
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2022-02-28 17:11:24 UTC

    
Comment 1 Blou 2022-02-28 17:18:10 UTC
Created attachment 131170 [details] [review]
Bug 30196: Odd number of elements warning caused by dt_from_string

Koha/DateUtils' dt_from_string is a sub expected to return a scalar, but
does not in all case.

In one instance, tt simply return... nothing, which cause one common call
output_pref( { dt => dt_from_string( $serinfo->{$d} ), dateonly => 1 } );
to output a warning.

Simply returning undef instead will solve the issue.
Comment 2 Aleisha Amohia 2022-04-21 04:33:02 UTC
Can you please attach a test plan?
Comment 3 Katrin Fischer 2022-06-25 16:07:18 UTC
Patch still applies, but missing test plan and unit test.

Blou, do you still intend to work on this one or shoudl we reset to NEW?
Comment 4 Blou 2022-06-27 12:50:10 UTC
I couldn't find a task here related to this bz.  I'll create on and have shi yao look at it.
Comment 5 Shi Yao Wang 2022-06-27 17:33:40 UTC
Created attachment 136656 [details] [review]
Bug 30196: Odd number of elements warning caused by dt_from_string

Koha/DateUtils' dt_from_string is a sub expected to return a scalar, but
does not in all case.

In one instance, tt simply return... nothing, which cause one common call
output_pref( { dt => dt_from_string( $serinfo->{$d} ), dateonly => 1 } );
to output a warning.

Simply returning undef instead will solve the issue.

Test plan:
1- Have a terminal tail intranet error logs
2- In the code, go to about.pl and change the only dt_from_string() to dt_from_string('0000-00-00')
3- Go to About Koha
4- Notice the logs output is: "Odd number of elements in anonymous hash at ..." and error 500 on the page.
5- Apply the patch
6- Redo step 3 and notice the previous log output is not there and the page loads
7- run `prove t/DateUtils.t` and all tests should pass
Comment 6 Shi Yao Wang 2022-06-27 17:35:16 UTC
Also notice this code in t/DateUtils.t:

># Return undef if passed mysql 0 dates
>$dt0 = dt_from_string( '0000-00-00', 'iso' );
>is( $dt0, undef, "undefined returned for 0 iso date" );

This means it is supposed to return undef. This patch only makes it clearly return undef instead of letting perl decide.
Comment 7 Jonathan Druart 2022-06-28 12:11:46 UTC
Instead use: output_pref({ str => $serinfo->{$d}, dateonly => 1 })
Comment 8 Jonathan Druart 2022-06-28 12:12:50 UTC
% perlcritic Koha/DateUtils.pm
"return" statement with explicit "undef" at line 61, column 5.  See page 199 of PBP.  (Severity: 5)
Comment 9 Blou 2022-06-28 13:02:05 UTC
Which relate to https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef which I find the explanation funny.  Yes, you might screw up boolean tests with returning an undef.  But this bug here demonstrate you can screw up hash creation.

Anyway, I have no problem using a standard.  But your solution avoids the problem while not fixing it.  Aren't you afraid somewhere someone will use it again the same way ?
Comment 10 Jonathan Druart 2022-06-28 13:22:02 UTC
We are trying to pass perlcritic, and it's a good idea to stick to it. We can easily workaround it, either using 'str' instead of 'dt' or adding 'scalar' to force the context.

I wouldn't bother fixing this bug because:
* There is bug 30718, and it would be way more interesting to work on it, and it should fix/remove most of the occurrences.
* You shouldn't have 0000-00-00 dates anymore in your DB (how did it happen?). This is a bug or a config issue on itself.

I stopped being afraid of Koha code, I sleep better now :D
Comment 11 Katrin Fischer 2023-06-17 15:54:02 UTC
(In reply to Jonathan Druart from comment #10)
> We are trying to pass perlcritic, and it's a good idea to stick to it. We
> can easily workaround it, either using 'str' instead of 'dt' or adding
> 'scalar' to force the context.
> 
> I wouldn't bother fixing this bug because:
> * There is bug 30718, and it would be way more interesting to work on it,
> and it should fix/remove most of the occurrences.
> * You shouldn't have 0000-00-00 dates anymore in your DB (how did it
> happen?). This is a bug or a config issue on itself.
> 
> I stopped being afraid of Koha code, I sleep better now :D

Moving to WONTFIX a per Joubu's comment and the fact that 30718 is now fixed. Please reopen if you disagree!