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.
Created attachment 21958 [details] [review] Proposed Patch As these cases do not need to access db have added a Biblio.t which test error returns and logs messages to stderr, to run prove t/Biblio.t
I'm happy to see that you're trying to address this problem. Can you please add a test plan to your commit message?
(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
Created attachment 22447 [details] [review] [SIGNED OFF] 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.
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>
Pushed to master. Thanks, Colin!
Patch pushed to 3.14.x, will be in 3.14.2.