Proposed coding standard: always check for object existence before use
Calling a method or accessing a member of an undefined object will throw a fatal error that looks like this: "can't call method * on an undefined value at ..." For example, this code will cause a software error if $data->{itemnumber} exists, but the item doesn't exist (maybe it's been deleted?): my $item = Koha::Items->find( $data->{itemnumber} ); my $biblio = $item->biblio; How this is handled will of course depend on context -- maybe we should carp and continue, or maybe just my $item = Koha::Items->find( $data->{itemnumber} ); my $biblio = $item->biblio if defined $item; I would like to add a coding standard that we always check for this. Thanks, --Barton
Hi Barton, Yes, and no. It depends. Example 1) my $biblio_to_edit = Koha::Biblios->find( $query->param('biblionumber') ); say sprintf "you are editing %s", $biblio_to_edit->title; Here we need to display a message if the biblio you want to edit does no longer exist (or invalid id). Example 2) sub one_more_sub { my ( $itemnumber ) = @_; my $item = Koha::Items->find( $itemnumber ); return unless $item; say $item->homebranch->branchname; say Koha::Frameworks->find($item->biblio->frameworkcode); } Here we do not need to make sure the homebranch or the biblio or the framework exist. The DB constraints take care of that for us. If the itemnumber exists, other objects exist. In that case of 1) we should stop the process at that point if the biblio does not exist, no need to continue. On bug 18403 I have introduced a way to do that, it is as much useful as ugly. We wanted to display a message if 1. the borrowernumber does not exist, or 2. the logged-in patron does not have the permission to view patron's info for a given borrowernumber. It looks like: my $logged_in_user = Koha::Patrons->find( $loggedinuser ) or die "Not logged in"; output_and_exit_if_error( $input, $cookie, $template, { module => 'members', logged_in_user => $logged_in_user, current_patron => $patron } ); What does it do? It dies in case $logged_in_user does not exist (it should *never* happen, but... who really knows?) And will render the template by setting an error flag "unknown_patron" or "cannot_see_patron_infos" if needed (for more info, see C4::Output::output_and_exit_if_error and blocking_errors.inc). Basically, we should avoid indentation blocks (if ( defined $item ) { # then continue }), it is usually a bad idea. See also: *Bug 20661* <https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20661> - Implement blocking errors for circulation scripts *Bug 20351* <https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20351> - Implement blocking errors for serials scripts Help me to make them in, and I will implement more. Going further, see a previous discussion "Expected behaviour if itemtype does not exist" - http://lists.koha-community.org/pipermail/koha-devel/2017-August/043924.html Cheers, Jonathan On Wed, 13 Jun 2018 at 14:23 Barton Chittenden <barton@bywatersolutions.com> wrote:
Calling a method or accessing a member of an undefined object will throw a fatal error that looks like this: "can't call method * on an undefined value at ..."
For example, this code will cause a software error if $data->{itemnumber} exists, but the item doesn't exist (maybe it's been deleted?):
my $item = Koha::Items->find( $data->{itemnumber} ); my $biblio = $item->biblio;
How this is handled will of course depend on context -- maybe we should carp and continue, or maybe just
my $item = Koha::Items->find( $data->{itemnumber} ); my $biblio = $item->biblio if defined $item;
I would like to add a coding standard that we always check for this.
Thanks,
--Barton _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
On Wed, Jun 13, 2018 at 3:07 PM, Jonathan Druart < jonathan.druart@bugs.koha-community.org> wrote:
Hi Barton,
Yes, and no. It depends.
... the dark side of TMTOWTDI ;-) I read through your examples, they all made good sense. Basically, we should avoid indentation blocks (if ( defined $item ) { #
then continue }), it is usually a bad idea.
I'm not quite clear on what you mean here... do you mean that we should either find a good object or fail gracefully? I'm going to give a more concrete example... this was new code in 17.11, but it's no longer in master... sub GetMemberAccountRecords { my ($borrowernumber) = @_; my $dbh = C4::Context->dbh; my @acctlines; my $numlines = 0; my $strsth = qq( SELECT * FROM accountlines WHERE borrowernumber=?); $strsth.=" ORDER BY accountlines_id desc"; my $sth= $dbh->prepare( $strsth ); $sth->execute( $borrowernumber ); my $total = 0; while ( my $data = $sth->fetchrow_hashref ) { if ( $data->{itemnumber} ) { my $item = Koha::Items->find( $data->{itemnumber} ); my $biblio = $item->biblio; Here, we're traversing accountlines... $data->{itemnumber} comes from accountlines. We find the item object based on that itemnumber, and then attempt to create a biblio record from that... which is fine, as long as the item hasn't been deleted. my $biblio = $item->biblio; is a fatal error in this case. I don't think that we're doing anything terribly consequential with $biblio... I totally agree that how we handle this depends on context... sometimes the instantiated object is critical, sometimes it's not... but I would assert that we *always* need to check to make sure that objects are defined before they're used. my $object = Koha::Foo->find( $something ); $object->{bar}; or my $object = Koha::Foo->new(); $object->{bar}; Without any error checking on $object should be a violation of coding standards... assuming that $object is defined simply isn't safe.
participants (2)
-
Barton Chittenden -
Jonathan Druart