Bug 39031 - ILL plugins should provide themselves the template path
Summary: ILL plugins should provide themselves the template path
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Pedro Amorim
URL:
Keywords: rel_24_05_candidate, rel_24_11_candidate
Depends on: 19605
Blocks:
  Show dependency treegraph
 
Reported: 2025-02-04 19:46 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-02-05 13:59 UTC (History)
2 users (show)

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


Attachments
Bug 39031: Use the backend's bundle_path() method (835 bytes, patch)
2025-02-05 13:14 UTC, Tomás Cohen Arazi (tcohen)
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 (tcohen) 2025-02-04 19:46:35 UTC
Right now, Koha assumes (in `Koha::ILL::Request->expand_template`) that `$plugin->new_ill_backend` returns a Koha::Plugin object that implements the required methods for acting like a proper ILL backend.

This is implied by the internal use of `bundle_path`:

```perl
    my $backend_plugin = $self->get_backend_plugin( $self->_backend->name );
    if ($backend_plugin) {

        # New way of loading backends: Through plugins
        $backend_dir  = $backend_plugin->bundle_path;
        $backend_tmpl = $backend_dir;
```

I'm writing a backend plugin that has a convenient `templates/` dir to better organize the many pieces. My `new_ill_backend()` method returns a new object that is designed to eventually inherit from `Koha::ILL::Backend` when time comes.

The current implementation assumes that everything is contained in the plugin's bundle path in a very specific way.

I propose we either allow the backend to tell where to look for templates:

```diff
-        $backend_dir  = $backend_plugin->bundle_path;
+        $backend_dir  = $self->_backend->template_path;
```

or implement a plugin method for the job:

```diff
-        $backend_dir  = $backend_plugin->bundle_path;
+        $backend_dir  = $backend_plugin->ill_template_path;
```

i.e. just implement a plugin method, that will return the path for querying the backend templates

I'd go with the first option, leaving it to the author's creativity to organize things the way they want.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-02-04 19:51:06 UTC
I feel like just changing it like this would be flexible enough, while keeping the current behavior for plugin-as-a-backend implementations:

```diff
-        $backend_dir  = $backend_plugin->bundle_path;
+        $backend_dir  = $self->_backend->bundle_path;
```

The only weird thing that will be left in this scenario, is that both `$backend_plugin` and `$self->_backend` are instances of the same plugin... just with some extra internal variables set by `new_ill_backend()`.

But in this context we already have an instance of the plugin, loaded 'as a backend' by `new_ill_backend()`. So it is redundant to instantiate the plugin, it seems.
Comment 2 Tomás Cohen Arazi (tcohen) 2025-02-05 13:14:19 UTC
Created attachment 177533 [details] [review]
Bug 39031: Use the backend's bundle_path() method

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi (tcohen) 2025-02-05 13:15:43 UTC
Assigning Pedro for QA to get his review.
Comment 4 Tomás Cohen Arazi (tcohen) 2025-02-05 13:59:23 UTC
And please backport :-D