Bug 11032 - Validate passed records in Biblio.pm
Summary: Validate passed records in Biblio.pm
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Colin Campbell
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-10 10:32 UTC by Colin Campbell
Modified: 2015-06-04 23:24 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (12.05 KB, patch)
2013-10-11 09:29 UTC, Colin Campbell
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11032: Check a valid MARC::Record passed to Biblio (12.34 KB, patch)
2013-10-27 09:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11032: Check a valid MARC::Record passed to Biblio (12.38 KB, patch)
2013-11-08 16:27 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2013-10-10 10:32:15 UTC
We've all seen Biblio.pm giving the error 'can't call method fields on an undefined value' for various reasons. They usually cause confusion or panic as a low level error gets thrown at the user with no apparent relation to their current operation. The problem is that many of Biblio's routines are rather optimistically written so that it thinks you've passed a MARC::Record object you have and it assumes that nothing could every go wrong in this best of all possible worlds.

Routines should at least check they have a defined record before trying to process it, and should log a stack trace so that the cause can be determined in the case of the error.

Merely passing a low level error to the end user is non-productive.
Comment 1 Colin Campbell 2013-10-11 09:29:03 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-10-11 12:23:05 UTC
I'm happy to see that you're trying to address this problem. Can you please add a test plan to your commit message?
Comment 3 Colin Campbell 2013-10-22 18:39:06 UTC
(In reply to Owen Leonard from comment #2)
> I'm happy to see that you're trying to address this problem. Can you please
> add a test plan to your commit message?

test plan is the tests added to t/Biblio.t see previous comment
Comment 4 Katrin Fischer 2013-10-27 09:21:14 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-11-08 16:27:54 UTC
Created attachment 22832 [details] [review]
Bug 11032: Check a valid MARC::Record passed to Biblio

Intermittently problems in the calling environment
cause a C4::Biblio to be called with and undefined
MARC::Record object. This results in the process
dieing and returning to the enduser a low level
message such as 'cannot call method x on an undefined
object'
For exported subroutines taking a MARC::Record object
check that object is defined otherwise return a logical
return value and log a stack trace to the error log.
A couple of cases were checking but dieing, this may have
unwelcome results in a persistent environment so croak has
been downgraded to carp

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Adds lots of checks for $record in various places, should
not affect behaviour.
Passed all tests and QA script, including new unit tests.
Tested adding and saving a new record.
Also tested detail and result pages without XSLT.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Galen Charlton 2013-12-10 05:04:47 UTC
Pushed to master.  Thanks, Colin!
Comment 7 Fridolin SOMERS 2013-12-24 10:51:35 UTC
Patch pushed to 3.14.x, will be in 3.14.2.