Bug 26555 - Add a way for Koha::Object(s) to carry execution information
Summary: Add a way for Koha::Object(s) to carry execution information
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 26515 26651 26652 26704
  Show dependency treegraph
 
Reported: 2020-09-28 12:13 UTC by Tomás Cohen Arazi
Modified: 2020-11-13 15:47 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
This enhancement adds a standardised way to pass execution information around within Koha::Objects. Execution data should be set by action methods calling `$self->add_message({ message => $message, type => $type, payload => $payload });` inside the action method. The caller can then access the execution data using `my $messages = $object->messages;`
Version(s) released in:
20.11.00


Attachments
Bug 26555: Add Koha::Object::Error (3.99 KB, patch)
2020-09-28 13:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26555: Add ->errors and ->add_error to Koha::Object (3.26 KB, patch)
2020-09-28 13:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26555: Add Koha::Object::Message (4.56 KB, patch)
2020-09-29 13:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26555: Add ->messages and ->add_message to Koha::Object (3.37 KB, patch)
2020-09-29 13:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26555: (QA follow-up) Add ->reset_messages (1.95 KB, patch)
2020-09-29 13:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26555: Add Koha::Object::Message (4.62 KB, patch)
2020-09-30 08:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 26555: Add ->messages and ->add_message to Koha::Object (3.42 KB, patch)
2020-09-30 08:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 26555: (QA follow-up) Add ->reset_messages (2.00 KB, patch)
2020-09-30 08:15 UTC, David Nind
Details | Diff | Splinter Review
Bug 26555: Add Koha::Object::Message (4.69 KB, patch)
2020-09-30 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26555: Add ->messages and ->add_message to Koha::Object (3.48 KB, patch)
2020-09-30 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26555: (QA follow-up) Add ->reset_messages (2.06 KB, patch)
2020-09-30 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26555: Make sure _messages is always initialized (1.93 KB, patch)
2020-09-30 12:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26555: Add Koha::Object::Message (4.69 KB, patch)
2020-10-09 17:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26555: Add ->messages and ->add_message to Koha::Object (3.48 KB, patch)
2020-10-09 17:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26555: Make sure _messages is always initialized (2.02 KB, patch)
2020-10-09 17:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26555: (QA follow-up) Add a 'payload' attribute for carrying useful things (2.55 KB, patch)
2020-10-15 12:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-09-28 12:13:00 UTC
I propose we add an ->errors method that returns a list of Koha::Object::Error objects. They would be added by using ->add_error($params). This way we can pass , for example, non-fatal error information to the caller.
Comment 1 Tomás Cohen Arazi 2020-09-28 13:54:24 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2020-09-28 13:54:33 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2020-09-28 16:22:09 UTC
Should we call it 'messages' to make it more general?

Jonathan pointed that this situation could be confusing:

    $o->bad_call
    $o->errors; # error is set
    $o->good_call
    $o->errors; # error is still set

Setting 'In discussion' with the hope to get some feedback. I think this pattern is something we would like to make use of. Or we could

    return $self, { something => else };

instead of crafting this into Koha::Object.
Comment 4 Martin Renvoize 2020-09-29 07:18:42 UTC
I'm in favour of such a standardised way of adding operational feedback for non-fatal errors/information passing.

We introduced such a use case in the Koha::Item trigger for when lost items are marked as found.  Having a standardised approach to passing such information around in the object would be great.. I've already found a few more cases for such feedback and it allows for continued moves toward fluent (chainable) interfaces.

Perhaps 'messages' is the best name for this as we already have exceptions for fatal errors, and I can see where Jonathan is coming from with your comment Tomas.
Comment 5 Tomás Cohen Arazi 2020-09-29 13:19:14 UTC
Created attachment 110911 [details] [review]
Bug 26555: Add Koha::Object::Message

This patch introduces a tiny class for encapsulating execution messages in
Koha::Object derived classes.

To test:
1. Apply this patch
2. Run;
   $ kshell
  k$ prove t/Koha/Object/Message.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 6 Tomás Cohen Arazi 2020-09-29 13:19:20 UTC
Created attachment 110912 [details] [review]
Bug 26555: Add ->messages and ->add_message to Koha::Object

This patch adds a way to make Koha::Object-derived classes to carry
messages around. This targets non-fatal errors, or around action flags
for the caller, to avoid complex exception handling on the controllers when
it is not expected to be fatal.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 7 Tomás Cohen Arazi 2020-09-29 13:19:25 UTC
Created attachment 110913 [details] [review]
Bug 26555: (QA follow-up) Add ->reset_messages
Comment 8 David Nind 2020-09-30 08:15:18 UTC
Created attachment 110957 [details] [review]
Bug 26555: Add Koha::Object::Message

This patch introduces a tiny class for encapsulating execution messages in
Koha::Object derived classes.

To test:
1. Apply this patch
2. Run;
   $ kshell
  k$ prove t/Koha/Object/Message.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2020-09-30 08:15:22 UTC
Created attachment 110958 [details] [review]
Bug 26555: Add ->messages and ->add_message to Koha::Object

This patch adds a way to make Koha::Object-derived classes to carry
messages around. This targets non-fatal errors, or around action flags
for the caller, to avoid complex exception handling on the controllers when
it is not expected to be fatal.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2020-09-30 08:15:28 UTC
Created attachment 110959 [details] [review]
Bug 26555: (QA follow-up) Add ->reset_messages

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Martin Renvoize 2020-09-30 08:40:27 UTC
Created attachment 110967 [details] [review]
Bug 26555: Add Koha::Object::Message

This patch introduces a tiny class for encapsulating execution messages in
Koha::Object derived classes.

To test:
1. Apply this patch
2. Run;
   $ kshell
  k$ prove t/Koha/Object/Message.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-09-30 08:40:31 UTC
Created attachment 110968 [details] [review]
Bug 26555: Add ->messages and ->add_message to Koha::Object

This patch adds a way to make Koha::Object-derived classes to carry
messages around. This targets non-fatal errors, or around action flags
for the caller, to avoid complex exception handling on the controllers when
it is not expected to be fatal.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-09-30 08:40:37 UTC
Created attachment 110969 [details] [review]
Bug 26555: (QA follow-up) Add ->reset_messages

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-09-30 08:42:07 UTC
I really like this implementation.  It works well and gives us a nice way to pass messages around inside our object system without throwing fatal exceptions for non-fatal actions.

Works well, contains tests.

Passing QA - Nice one Tomas!
Comment 15 Tomás Cohen Arazi 2020-09-30 12:43:35 UTC
Created attachment 110983 [details] [review]
Bug 26555: Make sure _messages is always initialized

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Jonathan Druart 2020-10-08 12:24:05 UTC
Tomas, I am not a fan of this, and I am wondering if the following approach would not be better.

Imagine you have one or more messages (ie. not blocking errors) to pass to the caller, then you could throw a Koha::Exception::Messages (or Koha::Exception::Errors?) that would contain a set of Koha::Exception.
That would allow us to tell the callers several things happened, and let it handle the situation how it wants.

What do you think?
Comment 17 Tomás Cohen Arazi 2020-10-08 13:03:00 UTC
(In reply to Jonathan Druart from comment #16)
> Tomas, I am not a fan of this, and I am wondering if the following approach
> would not be better.

I need to explain that this was just my attempt to find a more elegant way of doing this:

https://gitlab.com/koha-community/Koha/-/blob/master/Koha/Item.pm#L886

i.e. passing execution information to the caller, while keeping chainability on the methods.

That was the only example I found in which we did something similar-ish. I renamed them from Koha::Object::Error to Koha::Object::Message as well,  for that reason.

> Imagine you have one or more messages (ie. not blocking errors) to pass to
> the caller, then you could throw a Koha::Exception::Messages (or
> Koha::Exception::Errors?) that would contain a set of Koha::Exception.
> That would allow us to tell the callers several things happened, and let it
> handle the situation how it wants.
> 
> What do you think?
 
I prefer exceptions as well. But how would that look in the code?

My implementation means you need to catch error conditions in your sub, and add error messages to notify. Then you just return normally. And is up to the caller to do something with those errors/messages. basket.pl and closeorder.pl don't care much about them, they will, but I intended not to change behaviour yet.

With your proposal, you would need to accumulate the messages (maybe reusing this ->add_message method) and at the end of the method, check if there are messages, and throw an exception. It is not mutually exclusive with this implementation, actually. It is just a matter of how you want to do/use it.

Take the basket.pl example. It doesn't bother to notify errors (I'm not implying that's correct, but you could want that). Do you really want to enforce a try/catch block for catching the error messages? And what if there are other exceptions?


try {
    $object->the_method;
}
catch {
   if ( ref($->isa) eq 'Koha::Object::Messages' ) {
      $check_errors = 1;
   }
   elsif ( ref($_) eq 'Koha::Exception::XXX' ) {
      HANDLE_ERROR();
   }
   else {
      UNHANDLED_EXCEPTION();
   }
};

NORMAL_EXECUTION({ check_errors => $check_errors })
Comment 18 Tomás Cohen Arazi 2020-10-08 13:05:46 UTC
(In reply to Jonathan Druart from comment #16)
> Tomas, I am not a fan of this, and I am wondering if the following approach
> would not be better.
> 
> Imagine you have one or more messages (ie. not blocking errors) to pass to
> the caller, then you could throw a Koha::Exception::Messages (or
> Koha::Exception::Errors?) that would contain a set of Koha::Exception.
> That would allow us to tell the callers several things happened, and let it
> handle the situation how it wants.
> 
> What do you think?

If you think we have time to have it in master for the next release, I can work on removing this generic thing from Koha::Object and only keep the message encapsulating class (Koha::Object::Message), and introduce a Koha::Exception::Object::Messages exception that carries a list of Koha::Object::Message objects.
Comment 19 Tomás Cohen Arazi 2020-10-09 17:54:05 UTC
Created attachment 111426 [details] [review]
Bug 26555: Add Koha::Object::Message

This patch introduces a tiny class for encapsulating execution messages in
Koha::Object derived classes.

To test:
1. Apply this patch
2. Run;
   $ kshell
  k$ prove t/Koha/Object/Message.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Tomás Cohen Arazi 2020-10-09 17:54:13 UTC
Created attachment 111427 [details] [review]
Bug 26555: Add ->messages and ->add_message to Koha::Object

This patch adds a way to make Koha::Object-derived classes to carry
messages around. This targets non-fatal errors, or around action flags
for the caller, to avoid complex exception handling on the controllers when
it is not expected to be fatal.

To test:
1. Apply this patchset
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Tomás Cohen Arazi 2020-10-09 17:54:21 UTC
Created attachment 111428 [details] [review]
Bug 26555: Make sure _messages is always initialized

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Marcel de Rooy 2020-10-12 09:30:32 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> Jonathan pointed that this situation could be confusing:
> 
>     $o->bad_call
>     $o->errors; # error is set
>     $o->good_call
>     $o->errors; # error is still set

What did we do here now? Nothing yet.
Should we clear the array on calling ->errors ?
Comment 23 Jonathan Druart 2020-10-12 10:01:38 UTC
(In reply to Marcel de Rooy from comment #22)
> (In reply to Tomás Cohen Arazi from comment #3)
> > Jonathan pointed that this situation could be confusing:
> > 
> >     $o->bad_call
> >     $o->errors; # error is set
> >     $o->good_call
> >     $o->errors; # error is still set
> 
> What did we do here now? Nothing yet.
> Should we clear the array on calling ->errors ?

Then:
  $o->bad_call
  $o->good_call
  $o->errors; # error is set!
Comment 24 Martin Renvoize 2020-10-12 11:01:23 UTC
(In reply to Jonathan Druart from comment #23)
> (In reply to Marcel de Rooy from comment #22)
> > (In reply to Tomás Cohen Arazi from comment #3)
> > > Jonathan pointed that this situation could be confusing:
> > > 
> > >     $o->bad_call
> > >     $o->errors; # error is set
> > >     $o->good_call
> > >     $o->errors; # error is still set
> > 
> > What did we do here now? Nothing yet.
> > Should we clear the array on calling ->errors ?
> 
> Then:
>   $o->bad_call
>   $o->good_call
>   $o->errors; # error is set!

It's no longer 'errors' but 'messages'; We're not trying to catch "errors" but rather convey informational messages down the stack. For hard-stop errors we should continue to throw exceptions.  I don't think we should reset the 'messages' content personally.. you can always grab a fresh object if you wish to do so.
Comment 25 Tomás Cohen Arazi 2020-10-12 11:52:49 UTC
Please look at the in discussion follow-ups.
Comment 26 Tomás Cohen Arazi 2020-10-15 12:36:12 UTC
Created attachment 111723 [details] [review]
Bug 26555: (QA follow-up) Add a 'payload' attribute for carrying useful things

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Jonathan Druart 2020-10-15 13:25:14 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 28 Martin Renvoize 2020-10-15 14:45:47 UTC
Super excited by this one :)
Comment 29 Lucas Gass 2020-11-13 15:47:42 UTC
enhancement will not be backported to 20.05.x