From 2a9993b9a87da0bab324ce6699f4378d5d720a0e Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 11 Oct 2016 11:52:12 +0200 Subject: [PATCH] Bug 17425: Make Koha::Object raise exceptions This patch makes Koha::Object raise exceptions in the following situations: - When a non existent accessor is called - When a non existent property is tried to be updated using ->set On implementing this change, we introduce Koha::Exceptions::Object class to contain all Koha::Object-specific exception definitions. Unit tests for this change are introduced in t/db_dependent/Koha/Objects.t To test: - Apply the patches on master - Run: $ prove t/db_dependent/Koha/Objects.t => SUCCESS: Tests return green - Sign off Note: A followup introduces the dependency for Try::Tiny. It needs to be present for running the tests. --- Koha/Exceptions/Object.pm | 20 ++++++++++++++++++++ Koha/Object.pm | 9 ++++----- t/db_dependent/Koha/Objects.t | 32 ++++++++++++++++++++++++++++---- 3 files changed, 52 insertions(+), 9 deletions(-) create mode 100644 Koha/Exceptions/Object.pm diff --git a/Koha/Exceptions/Object.pm b/Koha/Exceptions/Object.pm new file mode 100644 index 0000000..e08b5a5 --- /dev/null +++ b/Koha/Exceptions/Object.pm @@ -0,0 +1,20 @@ +package Koha::Exceptions::Object; + +use Modern::Perl; + +use Exception::Class ( + + 'Koha::Exceptions::Object' => { + description => 'Something went wrong!', + }, + 'Koha::Exceptions::Object::MethodNotFound' => { + isa => 'Koha::Exceptions::Object', + description => "Invalid method", + }, + 'Koha::Exceptions::Object::PropertyNotFound' => { + isa => 'Koha::Exceptions::Object', + description => "Invalid property", + } +); + +1; diff --git a/Koha/Object.pm b/Koha/Object.pm index 9d23fb4..0dcbdca 100644 --- a/Koha/Object.pm +++ b/Koha/Object.pm @@ -22,6 +22,7 @@ use Modern::Perl; use Carp; use Koha::Database; +use Koha::Exceptions::Object; =head1 NAME @@ -185,8 +186,7 @@ sub set { foreach my $p ( keys %$properties ) { unless ( grep {/^$p$/} @columns ) { - carp("No property $p!"); - return 0; + Koha::Exceptions::Object::PropertyNotFound->throw( "No property $p for " . ref($self) ); } } @@ -273,10 +273,9 @@ sub AUTOLOAD { my $value = $self->_result()->get_column( $method ); return $value; } + } else { + Koha::Exceptions::Object::MethodNotFound->throw( "No method $method for " . ref($self) ); } - - carp "No method $method!"; - return; } =head3 _type diff --git a/t/db_dependent/Koha/Objects.t b/t/db_dependent/Koha/Objects.t index 46348e4..61aaefb 100644 --- a/t/db_dependent/Koha/Objects.t +++ b/t/db_dependent/Koha/Objects.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 7; +use Test::More tests => 8; use Test::Warn; use Koha::Authority::Types; @@ -29,8 +29,11 @@ use Koha::Database; use t::lib::TestBuilder; +use Try::Tiny; + my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; +my $builder = t::lib::TestBuilder->new; is( ref(Koha::Authority::Types->find('')), 'Koha::Authority::Type', 'Koha::Objects->find should work if the primary key is an empty string' ); @@ -40,7 +43,7 @@ is( $borrowernumber_exists, 1, 'Koha::Objects->columns should return the table c subtest 'update' => sub { plan tests => 2; - my $builder = t::lib::TestBuilder->new; + $builder->build( { source => 'City', value => { city_country => 'UK' } } ); $builder->build( { source => 'City', value => { city_country => 'UK' } } ); $builder->build( { source => 'City', value => { city_country => 'UK' } } ); @@ -60,7 +63,7 @@ subtest 'pager' => sub { subtest 'reset' => sub { plan tests => 1; - my $builder = t::lib::TestBuilder->new; + my $patrons = Koha::Patrons->search; my $first_borrowernumber = $patrons->next->borrowernumber; my $second_borrowernumber = $patrons->next->borrowernumber; @@ -69,7 +72,7 @@ subtest 'reset' => sub { subtest 'delete' => sub { plan tests => 2; - my $builder = t::lib::TestBuilder->new; + my $patron_1 = $builder->build({source => 'Borrower'}); my $patron_2 = $builder->build({source => 'Borrower'}); is( Koha::Patrons->search({ -or => { borrowernumber => [ $patron_1->{borrowernumber}, $patron_2->{borrowernumber}]}})->delete, 2, ''); @@ -81,5 +84,26 @@ subtest 'not_covered_yet' => sub { warning_is { Koha::Patrons->search->not_covered_yet } { carped => 'The method not_covered_yet is not covered by tests' }, "If a method is not covered by tests, the AUTOLOAD method won't execute the method"; }; +subtest 'Exceptions' => sub { + plan tests => 2; + + my $patron_borrowernumber = $builder->build({ source => 'Borrower' })->{ borrowernumber }; + my $patron = Koha::Patrons->find( $patron_borrowernumber ); + + try { + $patron->blah('blah'); + } catch { + ok( $_->isa('Koha::Exceptions::Object::MethodNotFound'), + 'Calling a non-existent method should raise a Koha::Exceptions::Object::MethodNotFound exception' ); + }; + + try { + $patron->set({ blah => 'blah' }); + } catch { + ok( $_->isa('Koha::Exceptions::Object::PropertyNotFound'), + 'Setting a non-existent property should raise a Koha::Exceptions::Object::PropertyNotFound exception' ); + }; +}; + $schema->storage->txn_rollback; 1; -- 2.10.1