Bug 35930 - ILL module broken if plugins disabled
Summary: ILL module broken if plugins disabled
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Andreas Jonsson
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 30719
Blocks:
  Show dependency treegraph
 
Reported: 2024-01-29 11:34 UTC by Hans Pålsson
Modified: 2024-02-15 09:52 UTC (History)
6 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:
24.05.00,23.11.03


Attachments
Bug 35930: Add guards for plugins_enabled (1.90 KB, patch)
2024-02-12 11:26 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 35930: Add guards for plugins_enabled (2.18 KB, patch)
2024-02-12 12:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35930: Add guards for plugins_enabled (2.24 KB, patch)
2024-02-12 14:52 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 Hans Pålsson 2024-01-29 11:34:06 UTC
When upgrading from 23.05 to 23.11.01 our ILL module (https://github.com/Libriotech/koha-illbackend-libris) stopped working until <enable_plugins> variable in koha-conf.xml was set to 1. Plack-error.log showed message Can't call method "GetPlugins" on an undefined value at /usr/share/koha/intranet/cgi-bin/ill/ill-requests.pl line 534.

Since the ILL-module is not a plugin this behaviour is unwanted.
Comment 1 Andreas Jonsson 2024-02-12 11:01:24 UTC
This is caused by the changes introduced in bug 30719:

# Get available metadata enrichment plugins
sub get_metadata_enrichment {
    my @candidates = Koha::Plugins->new()->GetPlugins({
        method => 'provides_api'
    });
    my @services = ();
    foreach my $plugin(@candidates) {
        my $supported = $plugin->provides_api();
        if ($supported->{type} eq 'search') {
            push @services, $supported;
        }
    }
    return \@services;
}
Comment 2 Andreas Jonsson 2024-02-12 11:26:11 UTC
Created attachment 162018 [details] [review]
Bug 35930: Add guards for plugins_enabled

There are ILL-backends that are not implemented as
Koha plubins and does not require plugins to be enabled.

The 'new' method in Koha::Plugins returns undefined if
plugins are disabled.  Therefore, calls to this method
must be guarded by a check that plugins actually are enabled.

Test plan:

* Code inspection of patch, alternatively
* Activate the ill system by installing a backend such as
  koha-illbackend-libris:
  https://github.com/Libriotech/koha-illbackend-libris
* Make sure plugins are disabled in koha-conf.xml
* In the staff interface, go to ILL requests.
* The page should load without getting an error 500.
Comment 3 Pedro Amorim 2024-02-12 12:20:27 UTC
Created attachment 162019 [details] [review]
Bug 35930: Add guards for plugins_enabled

The 'new' method in Koha::Plugins returns undefined if
plugins are disabled.  Therefore, calls to this method
must be guarded by a check that plugins actually are enabled.

Test plan:

* Code inspection of patch, alternatively
* Activate the ill system by installing a backend such as
  koha-illbackend-libris:
  https://github.com/Libriotech/koha-illbackend-libris
* Make sure plugins are disabled in koha-conf.xml
* In the staff interface, go to ILL requests.
* The page should load without getting an error 500.

PA amended commit message: This is not related to ILL backends being plugins or not
This is about ILL batches, where checking for metadata enrichment plugins was missing 'enable_plugins' guard
Additionally, unrelated to batches, it's also about ILLAvailability, where checking for ILL availabililty plugins was missing enable_plugins guard

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 4 Pedro Amorim 2024-02-12 12:25:29 UTC
Thanks @Andreas.
This makes sense, and I've tested to also make sure there are no regressions:
ILLCheckAvailability -> enabled, installed an availability plugin and tested with enable_plugins 0 and 1, all working as expected:
https://github.com/PTFS-Europe/koha-plugin-ill-avail-eds

Installed a metadata enrichment plugin, tested with ILL batches, with enable_plugins 0 and 1, all working as expected:
https://github.com/PTFS-Europe/koha-plugin-api-pubmed

"As expected" above means these features are not available if enable_plugins=0 and no errors occur.

If someone else SOs this, please convert my SO to PQA and update accordingly.
Comment 5 Pedro Amorim 2024-02-12 12:27:55 UTC
Upping this to "major" as it will cause an ugly 500 error and block ILL basic functionality for installations using ILLModule with enable_plugins=0.
Comment 6 Hans Pålsson 2024-02-12 13:41:44 UTC
Tested the patch and it works. I mark this as PQA per request, if that´s wrong I'll take the blame. :)
Comment 7 Tomás Cohen Arazi 2024-02-12 14:52:12 UTC
Created attachment 162038 [details] [review]
Bug 35930: Add guards for plugins_enabled

The 'new' method in Koha::Plugins returns undefined if
plugins are disabled.  Therefore, calls to this method
must be guarded by a check that plugins actually are enabled.

Test plan:

* Code inspection of patch, alternatively
* Activate the ill system by installing a backend such as
  koha-illbackend-libris:
  https://github.com/Libriotech/koha-illbackend-libris
* Make sure plugins are disabled in koha-conf.xml
* In the staff interface, go to ILL requests.
* The page should load without getting an error 500.

PA amended commit message: This is not related to ILL backends being plugins or not
This is about ILL batches, where checking for metadata enrichment plugins was missing 'enable_plugins' guard
Additionally, unrelated to batches, it's also about ILLAvailability, where checking for ILL availabililty plugins was missing enable_plugins guard

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2024-02-12 14:55:26 UTC
(In reply to Hans Pålsson from comment #6)
> Tested the patch and it works. I mark this as PQA per request, if that´s
> wrong I'll take the blame. :)

It never hurts to have more testers :-D Don't forget to stamp your signature on the patch, though.

```
git commit --amend -s --no-edit
```

Best regards
Comment 9 Martin Renvoize 2024-02-14 13:30:37 UTC
I added Hans Pålsson signoff line inline and have pushed to master for 24.05.00 to allow rmaints to backport for the next releases.

Thanks for your work everyone.
Comment 10 Fridolin Somers 2024-02-15 09:52:28 UTC
Pushed to 23.11.x for 23.11.03