From 71e7804d2c5b7576a86cfe2234cff34020742d77 Mon Sep 17 00:00:00 2001
From: Tomas Cohen Arazi <tomascohen@theke.io>
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.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
---
 Koha/Exceptions/Object.pm     | 20 ++++++++++++++++++++
 Koha/Object.pm                |  7 +++----
 t/db_dependent/Koha/Objects.t | 30 +++++++++++++++++++++++++++---
 3 files changed, 50 insertions(+), 7 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 0000000000..e08b5a5cd0
--- /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 c6bfede2c5..cb2b9ef4a4 100644
--- a/Koha/Object.pm
+++ b/Koha/Object.pm
@@ -23,6 +23,7 @@ use Modern::Perl;
 use Carp;
 
 use Koha::Database;
+use Koha::Exceptions::Object;
 
 =head1 NAME
 
@@ -170,8 +171,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) );
         }
     }
 
@@ -252,8 +252,7 @@ sub AUTOLOAD {
 
     my $r = eval { $self->_result->$method(@_) };
     if ( $@ ) {
-        carp "No method $method found for " . ref($self) . " " . $@;
-        return
+        Koha::Exceptions::Object::MethodNotFound->throw( "No method $method for " . ref($self) );
     }
     return $r;
 }
diff --git a/t/db_dependent/Koha/Objects.t b/t/db_dependent/Koha/Objects.t
index 87f4a94491..de7b327e0a 100644
--- a/t/db_dependent/Koha/Objects.t
+++ b/t/db_dependent/Koha/Objects.t
@@ -31,8 +31,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' );
 
@@ -42,7 +45,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' } } );
@@ -62,7 +65,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;
@@ -71,7 +74,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, '');
@@ -111,5 +114,26 @@ subtest 'search_related' => sub {
     is( $libraries[1]->branchcode, $patron_2->{branchcode}, 'Koha::Objects->search_related should work as expected' );
 };
 
+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.39.5