+100 :)

+100 too :-)

Lets make this happen for 23.11 :)

(In reply to Martin Renvoize from comment #3)
> Lets make this happen for 23.11 :)

+ 1 :)

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

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.

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

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

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

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!

I've just sent a PR for adjusting the sample plugin to the proposed changes.

Created attachment 164084 [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>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 164085 [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>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 164086 [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>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 164087 [details] [review]
Bug 19605: Tidy

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 164088 [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>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 164089 [details] [review]
Bug 19605: Clarify method name

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 164090 [details] [review]
Bug 19605: Fix after bug 35536

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 164091 [details] [review]
Bug 19605: Another fix after bug 35536

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

(In reply to Tomás Cohen Arazi from comment #29)
> I've just sent a PR for adjusting the sample plugin to the proposed changes.

I got some errors testing it and followed up, please take a look when possible:
https://github.com/PTFS-Europe/koha-ill-backend-plugin

(In reply to Pedro Amorim from comment #39)
> (In reply to Tomás Cohen Arazi from comment #29)
> > I've just sent a PR for adjusting the sample plugin to the proposed changes.
> 
> I got some errors testing it and followed up, please take a look when
> possible:
> https://github.com/PTFS-Europe/koha-ill-backend-plugin

You're rigth with the fix. Sorry for that!

Update to the test plan:
When installing the pluginBackend, please install version 2.0.5 here:
https://github.com/PTFS-Europe/koha-ill-backend-plugin/releases/tag/v2.0.5

It contains the latest CSRF compatibility changes required to create an ILL request on master for this backend.

Created attachment 165201 [details] [review]
Bug 19605: (QA follow-up): available_backends fix reduce

available_backends was only considering dir backends for reduce, it should consider all backends, including plugin backends.

This relates to the ILLOpacbackends syspref, where if you add a backend name that happens to be a plugin, it was not disabling it for the OPAC.
This change considers all backends for Illopacbackends, regardless of if its a plugin or a directory backend.

This was made evident while writing tests for bug 36197.
Regression tests for this particular use case, and others, are part of bug 36197.

Pushed for 24.05!

Well done everyone, thank you!

Not backported to 23.11.x

It'd be great to get some clear documentation on this, as conceptually the idea of providing ILL backends via Koha plugins sounds great.

I'm intrigued by the idea of an ISO 18626 ILL Koha plugin, although not sure if it would be that straightforward or if backends like Rapido and ReShare would need their own plugins. Waiting to hear back libraries about workflows...