Bug 19605 - ILL backends should be pluggable through regular Koha plugins
Summary: ILL backends should be pluggable through regular Koha plugins
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Pedro Amorim
QA Contact: Tomás Cohen Arazi
URL: https://github.com/PTFS-Europe/koha-i...
Keywords:
Depends on:
Blocks: 23578 35581
  Show dependency treegraph
 
Reported: 2017-11-09 16:54 UTC by Tomás Cohen Arazi
Modified: 2024-03-25 17:21 UTC (History)
7 users (show)

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


Attachments
Bug 19605: Add 'ill_backend' plugin category (2.12 KB, patch)
2023-10-30 14:46 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 19605: Update expandTemplate to expand_template method name (4.17 KB, patch)
2023-10-30 14:46 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 19605: Add support for ILL backends as plugins (7.52 KB, patch)
2023-10-30 14:46 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 19605: Add 'ill_backend' plugin category (2.17 KB, patch)
2024-01-02 19:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 19605: Update expandTemplate to expand_template method name (4.21 KB, patch)
2024-01-02 19:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 19605: Add support for ILL backends as plugins (7.56 KB, patch)
2024-01-02 19:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 19605: Add 'ill_backend' plugin category (2.17 KB, patch)
2024-03-09 13:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Update expandTemplate to expand_template method name (4.22 KB, patch)
2024-03-09 13:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Add support for ILL backends as plugins (7.57 KB, patch)
2024-03-09 13:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Tidy (1.34 KB, patch)
2024-03-09 13:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Rename method and use the instantiated plugin in the call (2.34 KB, patch)
2024-03-09 13:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Clarify method name (1.50 KB, patch)
2024-03-09 13:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Add 'ill_backend' plugin category (2.23 KB, patch)
2024-03-09 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Update expandTemplate to expand_template method name (4.27 KB, patch)
2024-03-09 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Add support for ILL backends as plugins (7.62 KB, patch)
2024-03-09 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Tidy (1.34 KB, patch)
2024-03-09 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Rename method and use the instantiated plugin in the call (2.34 KB, patch)
2024-03-09 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Clarify method name (1.50 KB, patch)
2024-03-09 13:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19605: Fix after bug 35536 (683 bytes, patch)
2024-03-25 17:21 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 2017-11-09 16:54:04 UTC
Right now, the ILL module requires a configuration entry for a path in which to look for the backend implementations. This should be implemented in a more simple way for end-users.
Comment 1 Katrin Fischer 2023-01-13 14:29:26 UTC
+100 :)
Comment 2 Michal Denar 2023-01-13 18:38:22 UTC
+100 too :-)
Comment 3 Martin Renvoize 2023-06-06 07:15:51 UTC
Lets make this happen for 23.11 :)
Comment 4 Katrin Fischer 2023-06-06 11:37:47 UTC
(In reply to Martin Renvoize from comment #3)
> Lets make this happen for 23.11 :)

+ 1 :)
Comment 5 Pedro Amorim 2023-10-30 14:46:38 UTC
Created attachment 158068 [details] [review]
Bug 19605: Add 'ill_backend' plugin category
Comment 6 Pedro Amorim 2023-10-30 14:46:42 UTC
Created attachment 158069 [details] [review]
Bug 19605: Update expandTemplate to expand_template method name
Comment 7 Pedro Amorim 2023-10-30 14:46:44 UTC
Created attachment 158070 [details] [review]
Bug 19605: Add support for ILL backends as plugins

This is designed to maintain existing functionality of loading ILL
backends through the backend_dir config (default Koha/Illbackends/).

A check for a plugin of a given backend name takes precedence over a
backend of the same name loaded the old way through backend_dir, this
means that if this happens, the backend plugin is used and NOT the
backend present in backend_dir.

Old backend_dir backends AND new backend plugins coexist.

Test plan, k-t-d:
1) Enable ILLModule and install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Install the plugin ILL backend example .kpz located at:
https://github.com/ammopt/koha-ill-backend-plugin/releases/tag/1.0.0
3) koha-plack --restart kohadev
4) Visit ILL requests:
/cgi-bin/koha/ill/ill-requests.pl
5) Click "+ New ILL request". Notice it lists 'FreeForm' and
   'PluginBackend'
6) Click 'PluginBackend'. Enter a '123' in pubmedid, '42' in cardnumber
   and pick a library. Click 'Marke request'
7) Notice the request is created successfully.
8) Visit plugins:
/cgi-bin/koha/plugins/plugins-home.pl
9) Click "View plugin by class". Pick "ill backend plugins". Notice the
   correct plugin is listed.
10) Click "configure" on the ILL backend plugin. Notice it's a normal
    plugin configuration page
Comment 8 Pedro Amorim 2023-10-30 14:48:05 UTC
Instructions on converting old ILL backend into a plugin can be found at the PluginBackend sample plugin https://github.com/ammopt/koha-ill-backend-plugin:
- Create your plugin, copy the contents from your old ILL backend into the plugin folder.
- Copy the code from your old Base.pm file into the plugin file named after the plugin.
- Rename the old backend 'metadata' method to 'backend_metadata'.
- Rename the old backend 'new' method to 'new_backend'.
Comment 9 Pedro Amorim 2023-10-30 14:49:58 UTC
My suggestions is that we support both ways of loading ILL backends (old backend_dir way and this new plugin way) for a while (1 year? 2 years?) and after that period we deprecate the old backend_dir way of doing things.
Comment 10 Katrin Fischer 2023-10-30 15:00:45 UTC
(In reply to Pedro Amorim from comment #9)
> My suggestions is that we support both ways of loading ILL backends (old
> backend_dir way and this new plugin way) for a while (1 year? 2 years?) and
> after that period we deprecate the old backend_dir way of doing things.

That sounds like a good plan. We should make sure the deprecation warning is well visible, in the Relase notes and on the wiki page and maybe the manual?
Comment 11 Magnus Enger 2023-10-31 07:16:02 UTC
(In reply to Pedro Amorim from comment #9)
> My suggestions is that we support both ways of loading ILL backends (old
> backend_dir way and this new plugin way) for a while (1 year? 2 years?) and
> after that period we deprecate the old backend_dir way of doing things.

That sounds like a most excellent plan.
Comment 12 David Nind 2024-01-02 19:46:38 UTC
Created attachment 160433 [details] [review]
Bug 19605: Add 'ill_backend' plugin category

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2024-01-02 19:46:41 UTC
Created attachment 160434 [details] [review]
Bug 19605: Update expandTemplate to expand_template method name

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2024-01-02 19:46:44 UTC
Created attachment 160435 [details] [review]
Bug 19605: Add support for ILL backends as plugins

This is designed to maintain existing functionality of loading ILL
backends through the backend_dir config (default Koha/Illbackends/).

A check for a plugin of a given backend name takes precedence over a
backend of the same name loaded the old way through backend_dir, this
means that if this happens, the backend plugin is used and NOT the
backend present in backend_dir.

Old backend_dir backends AND new backend plugins coexist.

Test plan, k-t-d:
1) Enable ILLModule and install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Install the plugin ILL backend example .kpz located at:
https://github.com/ammopt/koha-ill-backend-plugin/releases/tag/1.0.0
3) koha-plack --restart kohadev
4) Visit ILL requests:
/cgi-bin/koha/ill/ill-requests.pl
5) Click "+ New ILL request". Notice it lists 'FreeForm' and
   'PluginBackend'
6) Click 'PluginBackend'. Enter a '123' in pubmedid, '42' in cardnumber
   and pick a library. Click 'Marke request'
7) Notice the request is created successfully.
8) Visit plugins:
/cgi-bin/koha/plugins/plugins-home.pl
9) Click "View plugin by class". Pick "ill backend plugins". Notice the
   correct plugin is listed.
10) Click "configure" on the ILL backend plugin. Notice it's a normal
    plugin configuration page

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Tomás Cohen Arazi 2024-02-12 17:47:23 UTC
I been looking at the PluginBackend implementation, and got a bit confused about this:

sub new_backend {
    my ( $class, $params ) = @_;

    my $self = {};

    $self->{_logger} = $params->{logger} if ( $params->{logger} );
    $self->{_config} = $params->{config} if ( $params->{config} );

    bless( $self, $class );

    return $self;
}


It seems to me that in this particular implementation, as we already have a plugin instance in this context:

+    my $backend_plugin = $self->get_backend_plugin($backend_name);
+    if ($backend_plugin) {
+
+        # New way of loading backends: Through plugins
+        my $backend_plugin_class = $backend_plugin->{class};
+
+        $self->{_my_backend} = $backend_plugin_class->new_backend(

it doesn't make sense to instantiate the plugin again. Thus, the PluginBackend implementation could just be:

sub new_backend {
    my ($self,$params) = @_;

    $self->{_logger} = $params->{logger} if ( $params->{logger} );
    $self->{_config} = $params->{config} if ( $params->{config} );

    return $self;
}

I would rather rename the method to `get_ill_backend`, and leave the plugin authors the decision on what to return (the name change is a matter of taste, but I think `ill` needs to be part of the method name).

In my case, I would love to be able to (say) have my own class for the backend, so  my implementation would be written like this:

sub get_ill_backend {
    my ($self,$params) = @_;

    require INNReach::ILL::Backend;

    return INNReach::ILL::Backend->new({
        plugin => $self,
        logger => $params->{logger}
    });
}
Comment 16 Tomás Cohen Arazi 2024-03-09 13:07:07 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2024-03-09 13:07:10 UTC Comment hidden (obsolete)
Comment 18 Tomás Cohen Arazi 2024-03-09 13:07:13 UTC Comment hidden (obsolete)
Comment 19 Tomás Cohen Arazi 2024-03-09 13:07:16 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2024-03-09 13:07:19 UTC Comment hidden (obsolete)
Comment 21 Tomás Cohen Arazi 2024-03-09 13:07:23 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2024-03-09 13:08:07 UTC
Created attachment 163002 [details] [review]
Bug 19605: Add 'ill_backend' plugin category

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2024-03-09 13:08:10 UTC
Created attachment 163003 [details] [review]
Bug 19605: Update expandTemplate to expand_template method name

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2024-03-09 13:08:13 UTC
Created attachment 163004 [details] [review]
Bug 19605: Add support for ILL backends as plugins

This is designed to maintain existing functionality of loading ILL
backends through the backend_dir config (default Koha/Illbackends/).

A check for a plugin of a given backend name takes precedence over a
backend of the same name loaded the old way through backend_dir, this
means that if this happens, the backend plugin is used and NOT the
backend present in backend_dir.

Old backend_dir backends AND new backend plugins coexist.

Test plan, k-t-d:
1) Enable ILLModule and install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Install the plugin ILL backend example .kpz located at:
https://github.com/ammopt/koha-ill-backend-plugin/releases/tag/1.0.0
3) koha-plack --restart kohadev
4) Visit ILL requests:
/cgi-bin/koha/ill/ill-requests.pl
5) Click "+ New ILL request". Notice it lists 'FreeForm' and
   'PluginBackend'
6) Click 'PluginBackend'. Enter a '123' in pubmedid, '42' in cardnumber
   and pick a library. Click 'Marke request'
7) Notice the request is created successfully.
8) Visit plugins:
/cgi-bin/koha/plugins/plugins-home.pl
9) Click "View plugin by class". Pick "ill backend plugins". Notice the
   correct plugin is listed.
10) Click "configure" on the ILL backend plugin. Notice it's a normal
    plugin configuration page

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2024-03-09 13:08:16 UTC
Created attachment 163005 [details] [review]
Bug 19605: Tidy

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2024-03-09 13:08:20 UTC
Created attachment 163006 [details] [review]
Bug 19605: Rename method and use the instantiated plugin in the call

This patch makes the backend instantiation happen through a plugin
object method instead of a static one. This way, the (already)
instantiated plugin can be reused (if needed) in the plugin workflow
like this:

sub new_il_backend {
    my ($self, $params) = @_;
    return Custom::Backend->new(
        {
            config => $params->{config},
            logger => $params->{logger},
            plugin => $self,
        }
    );
}

The passed plugin object would then be used to retrieve whatever
plugin-defined configurations, template paths or even helper methods. On
INN-Reach, it gives the backend access to API user agents, task queue
injection, etc.

This patch also renames the method so it is more straight-forward this is
related to ILL.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2024-03-09 13:08:23 UTC
Created attachment 163007 [details] [review]
Bug 19605: Clarify method name

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2024-03-09 13:13:15 UTC
Sorry for the noise, I forgot to stamp my signature.

Remarks:

* I will submit on a follow-up bug an idea for some error checking we are lacking here. No rush though. Will do on monday for sure.
* I didn't understand why you changed the expandTemplate method name here. No problem for me but we usually do it on a separate report.
* I'm not sure we should tag 'this is the new way' in the code, as it might be confusing for readers a couple months later. I fixed this in one place, making the code and comments assume 'plugins is the way' and the 'old one' is a 'fallback' mechanism. I prefer such terminology.

This all looks great. Awesome job! Sorry for the delay!
Comment 29 Tomás Cohen Arazi 2024-03-09 13:24:50 UTC
I've just sent a PR for adjusting the sample plugin to the proposed changes.
Comment 30 Tomás Cohen Arazi 2024-03-25 17:21:02 UTC
Created attachment 163846 [details] [review]
Bug 19605: Fix after bug 35536

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