https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 56917 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56917&action=edit Bug 17502: Resolve internal server error on 0000-00-00 (with Plack) This patch makes the following changes: [1] In Koha/DateUtils.pm, sub output_pref: Add a test if $dt is really a DateTime before calling method ymd. Preventing a crash like: Can't locate object method "ymd" via package "dateonly" Change [2] fixes a specific call where this went wrong, but this test may still catch one or two other occurrences in the code. [2] In C4/Search.pm, sub searchResults: Scalarize the return value of dt_from_string as a hash parameter for output_pref. Like: { dt => scalar dt_from_string( $date ), dateonly => 1 } If the sub would return undef (without adding scalar), the hash would have an odd number of elements. (A warning is suppressed in C4/Search.) For a value of 0000-00-00 in item.onloan, this resolves an internal server error on a search with this item in the results. [3] Adds a few unit tests in t/DateUtils.t. Note: I opened up another report to prevent having 000-00-00 in item dates. Test plan: [1] Without this patch: Put a 0000-00-00 in items.onloan for one item. (Make 952 $q visible and just enter some text in it.) [2] Verify that a search on opac/intranet that includes this item, results in a crash. (Plack will just say: Internal server error.) NOTE: With Jessie and Plack, the search crashes. On a Wheezy without Plack, it did not crash. [3] Apply this patch. (Restart Plack.) [4] Verify that the same search now does not fail. [5] Run the adjusted unit test t/DateUtils.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56916|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I am wondering about the severity of this patch. Somehow we had a number of items with 0000-00-00 in onloan. (Dating from older Koha versions.) You could argue that this would be exceptional. But still Koha should not crash on such a detail. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Did you try the str param of output_pref? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- We should favour the use of output_pref({ str => $date_string }); instead of output_pref({ dt => dt_from_string( $date_string ) }); That said I agree that the new check in output_pref makes sense. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 57597 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57597&action=edit Bug 17502: [Follow-up] Better use the str parameter Constructions like output_pref({ dt => dt_from_string( $str ), ... could be simplified with output_pref({ str => $str, ... Test plan: See previous plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=15822 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Closed -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to M. Tompsett from comment #12)

(In reply to Marcel de Rooy from comment #9)

...

Main change has been used in 15822. So forget the rest.

Well, unless you care about type checking, if you actually right your test, you will still trigger an error, because dt is a string and not a DateTime. A quick perusal of the code shows lots of dt_from_string calls, and lots of $foo's used. Given the number of $foo's, I suspect they probably are dates, because otherwise we would have hit this problem sooner. So leaving it closed, because type checking the programmer's code should be part of the QA process. :)

Probably you make a point. Coming back later.. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Resolve internal server |Add type check to |error on 0000-00-00 (with |output_pref |Plack) | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56917|0 |1 is obsolete| | Attachment #57597|0 |1 is obsolete| | --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 61458 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61458&action=edit Bug 17502: Add type check to output_pref This patch makes the following changes: [1] In Koha/DateUtils.pm, sub output_pref: Add a test if $dt is really a DateTime before calling method ymd. Preventing a crash like: Can't locate object method "ymd" via package "dateonly". See also BZ 17502/15822. [2] Adds a few unit tests in t/DateUtils.t. Test plan: [1] Run the adjusted unit test t/DateUtils.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Hum, not sure about that patch. Of course it is what we should do, but it will hide possible errors in our codebase. If the app crashes because the call is wrong, at least we know there is something wrong. With this patch we are going to hide the problem. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Marcel de Rooy from comment #17)

Throw an exception

Yeah. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 61510 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61510&action=edit Bug 17502: Throw some exceptions in output_pref This patch adds Koha/Exceptions/DateUtils.pm. It contains now 4 exceptions for output_pref: [1] default general exception, [2] conflicting parameters (dt and str), [3] invalid date string, [4] no date time object. Test plan: Run the adjusted t/DateUtils.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #61510|0 |1 is obsolete| | --- Comment #21 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 61519 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61519&action=edit [SIGNED-OFF] Bug 17502: Throw some exceptions in output_pref This patch adds Koha/Exceptions/DateUtils.pm. It contains now 4 exceptions for output_pref: [1] default general exception, [2] conflicting parameters (dt and str), [3] invalid date string, [4] no date time object. Test plan: Run the adjusted t/DateUtils.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Hi Marcel, The classes of the exceptions should not be named with the module they are raised from. Also I do not think we need 1 class per exception. If you want to raise an exception because the API has not been used correctly I guess we need (or have already?) a generic exception for it. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #23)

The classes of the exceptions should not be named with the module they are raised from.

Hmm. Just following what we already did (and approved). Like: package Koha::Patron::Modification; use Koha::Exceptions::Patron::Modification; Koha::Exceptions::Patron::Modification::DuplicateVerificationToken->throw === package Koha::Exceptions::Patron::Modification; 'Koha::Exceptions::Patron::Modification::DuplicateVerificationToken' => { isa => 'Koha::Exceptions::Patron::Modification', description => "The verification token given already exists" }, === Please explain. Should we formulate a coding guideline here? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #27 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Marcel de Rooy from comment #25)

(In reply to Jonathan Druart from comment #23)

...

The classes of the exceptions should not be named with the module they are raised from.

Hmm. Just following what we already did (and approved). Like:

package Koha::Patron::Modification; use Koha::Exceptions::Patron::Modification; Koha::Exceptions::Patron::Modification::DuplicateVerificationToken->throw === package Koha::Exceptions::Patron::Modification;

'Koha::Exceptions::Patron::Modification::DuplicateVerificationToken' => { isa => 'Koha::Exceptions::Patron::Modification', description => "The verification token given already exists" }, ===

Please explain. Should we formulate a coding guideline here?

We might need to. I think the thing with the exceptions you add is: - There's no need to have them tied to output_pref, they could jus tbe top-level for the DateUtils namespace. Remember when you catch them, you have the context in which it happened and can build the needed actions. - The ones you add should be general exceptions: WrongParameters, MissingParameters, InvalidParamType. Keep in mind that you can use them like this: Koha::Exceptions::InvalidParamType->throw('DateTime object expected'); Koha::Exceptions::WrongParameters->throw('Conflicting parameters dt and str passed'); instead of baking a too specific exception. Regarding the Patron modification ones, I found that patron modification tokens were too specific, and they deserved its own exception. But it might be subject to future changes if someone raises it. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 62169 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62169&action=edit Bug 17502: Throw some exceptions in output_pref Test plan: Run the adjusted t/DateUtils.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> NOTE: This patch is amended in QA. The exceptions are moved from a separate module to the general section of Exceptions.pm. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #31 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 62409 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62409&action=edit Bug 17502: Throw some exceptions in output_pref Test plan: Run the adjusted t/DateUtils.t Signed-off-by: Josef Moravec <josef.moravec@gmail.com> NOTE: This patch is amended in QA. The exceptions are moved from a separate module to the general section of Exceptions.pm. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #33 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I am expecting explosions with these patches but that will help to discover hidden bugs. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |18473 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18473 [Bug 18473] Search broken due to bug in dates handling -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17502 --- Comment #36 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Katrin Fischer from comment #35)

Hm, expecting explosions... should this be in stable?

I don't think so! Unless it fixes something, but it is too early to say it doesn't break things. -- You are receiving this mail because: You are watching all bug changes.