Summary: | Odd number of elements warning caused by dt_from_string | ||
---|---|---|---|
Product: | Koha | Reporter: | Blou <philippe.blouin> |
Component: | Architecture, internals, and plumbing | Assignee: | Blou <philippe.blouin> |
Status: | CLOSED WONTFIX | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | aleisha, jonathan.druart, philippe.blouin, shi-yao.wang |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 25790 | ||
Attachments: |
Bug 30196: Odd number of elements warning caused by dt_from_string
Bug 30196: Odd number of elements warning caused by dt_from_string |
Description
Blou
2022-02-28 17:11:24 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. Can you please attach a test plan? 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? I couldn't find a task here related to this bz. I'll create on and have shi yao look at it. 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 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.
Instead use: output_pref({ str => $serinfo->{$d}, dateonly => 1 }) % perlcritic Koha/DateUtils.pm "return" statement with explicit "undef" at line 61, column 5. See page 199 of PBP. (Severity: 5) 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 ? 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 (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! |