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.

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.

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

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

Created attachment 110913 [details] [review]
Bug 26555: (QA follow-up) Add ->reset_messages

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>

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>

Created attachment 110959 [details] [review]
Bug 26555: (QA follow-up) Add ->reset_messages

Signed-off-by: David Nind <david@davidnind.com>

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>

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>

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>

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!

Created attachment 110983 [details] [review]
Bug 26555: Make sure _messages is always initialized

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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?

(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 })

(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.

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>

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>

Created attachment 111428 [details] [review]
Bug 26555: Make sure _messages is always initialized

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

(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 ?

(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!

(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.

Please look at the in discussion follow-ups.

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>

Pushed to master for 20.11, thanks to everybody involved!

Super excited by this one :)

enhancement will not be backported to 20.05.x