Bug 32107 - Send $self to Plugins->call and call discard_changes inside
Summary: Send $self to Plugins->call and call discard_changes inside
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-11-04 09:07 UTC by Marcel de Rooy
Modified: 2022-11-04 09:45 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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.