Created attachment 56916 [details] [review] Bug 17502: Resolve internal server error on 0000-00-00 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.) [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
Created attachment 56917 [details] [review] 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
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.
Did you try the str param of output_pref?
(In reply to Jonathan Druart from comment #4) > Did you try the str param of output_pref? Now. Seems to work too. But the changes here add some more tests.
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.
(In reply to Jonathan Druart from comment #6) > 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. I will add a follow-up
Created attachment 57597 [details] [review] 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>
Main change has been used in 15822. So forget the rest.
(In reply to Marcel de Rooy from comment #9) > Main change has been used in 15822. > So forget the rest. What is the status then?
Closed
(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. :)
(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..
Created attachment 61458 [details] [review] 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
Well, Mark, go ahead and sign off this one ;)
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.
Throw an exception
(In reply to Marcel de Rooy from comment #17) > Throw an exception Yeah.
Created attachment 61510 [details] [review] 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
Created attachment 61518 [details] [review] [SIGNED-OFF] 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 Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 61519 [details] [review] [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>
(In reply to Josef Moravec from comment #21) > Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Thanks Josef
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.
Moreover I do not know if it is very safe to explode if output_pref is called without a defined variable. I can easily imagine some places where output_pref($var) is called with $var undefined.
(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?
(In reply to Jonathan Druart from comment #24) > Moreover I do not know if it is very safe to explode if output_pref is > called without a defined variable. > I can easily imagine some places where output_pref($var) is called with $var > undefined. Agreed. Changing status.
(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.
Created attachment 62168 [details] [review] 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 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 62169 [details] [review] 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>
Created attachment 62408 [details] [review] 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 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 62409 [details] [review] 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>
Created attachment 62410 [details] [review] Bug 17502: Add info when throwing the exception Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I am expecting explosions with these patches but that will help to discover hidden bugs.
Pushed to master for 17.05, thanks Marcel, Jonathan!
Hm, expecting explosions... should this be in stable?
(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.
Thx Tomas! This won't get ported back to 16.11.x as it is an enhancement.