Bug 32107

Summary:	Send $self to Plugins->call and call discard_changes inside
Product:	Koha	Reporter:	Marcel de Rooy <m.de.rooy>
Component:	Plugin architecture	Assignee:	Bugs List <koha-bugs>
Status:	NEW ---	QA Contact:	Testopia <testopia>
Severity:	enhancement
Priority:	P5 - low	CC:	dcook, jonathan.druart, julian.maurice, tomascohen
Version:	Main
Hardware:	All
OS:	All
Change sponsored?:	---	Patch complexity:	---
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:
Circulation function:

Description Marcel de Rooy 2022-11-04 09:07:28 UTC

Resulting from a discussion on IRC and in the QA team. If there is a need for further discussion, we can continue here.

The idea is: We should no longer $self->store and pass $self->get_from_storage to the ->call routine.
But we should $self->store, pass $self to Plugins->call, and ->discard_changes once inside ->call to fetch timestamps and default values from DBMS.

What happens after ->call? Currently, we pass a copy but continue afterwards with the old $self. Normally, we wont do much more, but this might create side-effects since we do not operate on the latest data (we ignore changes from plugins).
If we pass $self, we have the latest state back after ->call. Though we are not sure if the plugin(s) did a store. If we would do a discard after the plugins loop in ->call, we actually ignore unsaved changes like we do now too and we do not disturb the plugins loop inside ->call.

To make it even more explicit, we could add a parameter to ->call that controls if we do or do not discard after the plugins loop..

Side note: The execution order of plugins seems to be database insert order when I look at the code in GetPlugins. Theoretically I would rather control that differently but that could be a topic of another report. Not in this scope ;)

Comment 1 Julian Maurice 2022-11-04 09:28:14 UTC

(In reply to Marcel de Rooy from comment #0)
> To make it even more explicit, we could add a parameter to ->call that
> controls if we do or do not discard after the plugins loop..

I think that Koha::Plugin::call should not do anything on its @args except passing it to plugins. It should not even care of what's in these @args.
IMO its the responsibility of the caller of Koha::Plugin::call to pass the right arguments and handle the eventual side-effects correctly.

Comment 2 Marcel de Rooy 2022-11-04 09:28:59 UTC

Adding a diff here from 31894

From 59ff56f2b77ed9b529e886b40bc2da63fb929a81 Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Tue, 25 Oct 2022 14:45:57 +0000
Subject: [PATCH] Bug 31894: (QA follow-up) Remove unneeded get_from_storage
 calls
Content-Type: text/plain; charset=utf-8

Test plan:
Run test again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
---
 Koha/Hold.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Koha/Hold.pm b/Koha/Hold.pm
index 8379e3f718..36b434ed2b 100644
--- a/Koha/Hold.pm
+++ b/Koha/Hold.pm
@@ -204,7 +204,7 @@ sub set_transfer {
         'after_hold_action',
         {
             action  => 'transfer',
-            payload => { hold => $self->get_from_storage }
+            payload => { hold => $self }
         }
     );
 
@@ -269,7 +269,7 @@ sub set_waiting {
         'after_hold_action',
         {
             action  => 'waiting',
-            payload => { hold => $self->get_from_storage }
+            payload => { hold => $self }
         }
     );
 
@@ -365,7 +365,7 @@ sub set_processing {
         'after_hold_action',
         {
             action  => 'processing',
-            payload => { hold => $self->get_from_storage }
+            payload => { hold => $self }
         }
     );
 
-- 
2.30.2

Comment 3 Marcel de Rooy 2022-11-04 09:32:21 UTC

(In reply to Julian Maurice from comment #1)
> (In reply to Marcel de Rooy from comment #0)
> > To make it even more explicit, we could add a parameter to ->call that
> > controls if we do or do not discard after the plugins loop..
> 
> I think that Koha::Plugin::call should not do anything on its @args except
> passing it to plugins. It should not even care of what's in these @args.
> IMO its the responsibility of the caller of Koha::Plugin::call to pass the
> right arguments and handle the eventual side-effects correctly.

Yes, I appreciate that principle. But the problem is that we are inserting more and more calls to Plugins->call and that we are loosing consistency. So we are looking for a balance here..

Comment 4 Julian Maurice 2022-11-04 09:42:47 UTC

> Yes, I appreciate that principle. But the problem is that we are inserting
> more and more calls to Plugins->call and that we are loosing consistency. So
> we are looking for a balance here..

Maybe adding a "wrapper subroutine" around `call` would work ? It would keep the `call` subroutine simple and data-agnostic while removing the boilerplate eventually needed everywhere Koha::Objects are passed to plugins.

Comment 5 Marcel de Rooy 2022-11-04 09:45:43 UTC

(In reply to Julian Maurice from comment #4)
> > Yes, I appreciate that principle. But the problem is that we are inserting
> > more and more calls to Plugins->call and that we are loosing consistency. So
> > we are looking for a balance here..
> 
> Maybe adding a "wrapper subroutine" around `call` would work ? It would keep
> the `call` subroutine simple and data-agnostic while removing the
> boilerplate eventually needed everywhere Koha::Objects are passed to plugins.

Good idea.