Bug 37872

Summary: ILL module has issues when plugins are disabled (enable_plugins = 0)
Product: Koha Reporter: Andreas Jonsson <andreas.jonsson>
Component: Plugin architectureAssignee: Andreas Jonsson <andreas.jonsson>
Status: Pushed to main --- QA Contact: Pedro Amorim <pedro.amorim>
Severity: major    
Priority: P3 CC: david, jonathan.druart, martin.renvoize, pedro.amorim
Version: 24.05   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes an issue when plugins are not enabled and the ILL module is enabled. This caused an error on the About Koha > System information section. This also fixes a page error shown when accessing the ILL module with enable_plugins = 0 in koha-conf.xml.
Version(s) released in:
24.11.00
Circulation function:
Attachments: Bug 37872: Add guards for enable_plugins
Bug 37872: Add guards for enable_plugins
Bug 37872: Add tests
Bug 37872: Add guards for enable_plugins
Bug 37872: Consider enable_plugins = 0 when fetching plugin backends
Bug 37872: Add tests
Bug 37872: Add guards for enable_plugins
Bug 37872: Consider enable_plugins = 0 when fetching plugin backends
Bug 37872: Add tests
Bug 37872: Add guards for enable_plugins
Bug 37872: Consider enable_plugins = 0 when fetching plugin backends

Description Andreas Jonsson 2024-09-09 18:13:27 UTC
This is similar to bug 35930.
Comment 1 Andreas Jonsson 2024-09-09 18:15:31 UTC
Created attachment 171227 [details] [review]
Bug 37872: Add guards for enable_plugins

There are ILL-backends that are not implemented as
Koha plugins 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.

Testplan for koha-testing-docker:

- Edit /etc/koha/sites/kohadev/koha-conf.xml and disable plugins by
  setting <enable_plugins>0</enable_plugins>
- sudo koha-mysql kohadev -e "UPDATE systempreferences SET value='1' WHERE variable='ILLModule';"
- Run restart_all.
- In staff interface, go to About koha -> System information. The page
  should load normally (as opposed to returning 500 internal server
  error).
Comment 2 David Nind 2024-09-09 21:46:36 UTC
Created attachment 171237 [details] [review]
Bug 37872: Add guards for enable_plugins

There are ILL-backends that are not implemented as
Koha plugins 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.

Testplan for koha-testing-docker:

- Edit /etc/koha/sites/kohadev/koha-conf.xml and disable plugins by
  setting <enable_plugins>0</enable_plugins>
- sudo koha-mysql kohadev -e "UPDATE systempreferences SET value='1' WHERE variable='ILLModule';"
- Run restart_all.
- In staff interface, go to About koha -> System information. The page
  should load normally (as opposed to returning 500 internal server
  error).

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2024-09-09 21:49:02 UTC
Testing notes (using KTD):

1. I tested on main.
2. I updated the assignee
Comment 4 Jonathan Druart 2024-09-11 13:12:17 UTC
git grepped to avoid yet another bug and found:

Koha/ILL/Request.pm
 408 sub get_backend_plugin {
 409     my ( $self, $backend_id ) = @_;
 410 
 411     my @backend_plugins = Koha::Plugins->new()->GetPlugins(

Not relevant as well?
Comment 5 Pedro Amorim 2024-10-29 10:24:47 UTC
Looking here
Comment 6 Pedro Amorim 2024-10-29 11:24:04 UTC
Created attachment 173617 [details] [review]
Bug 37872: Add tests

$ prove t/db_dependent/Koha/ILL/Request.t
$ prove t/db_dependent/Koha/ILL/Request/Config.t

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 7 Pedro Amorim 2024-10-29 11:24:06 UTC
Created attachment 173618 [details] [review]
Bug 37872: Add guards for enable_plugins

There are ILL-backends that are not implemented as
Koha plugins 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.

Testplan for koha-testing-docker:

- Edit /etc/koha/sites/kohadev/koha-conf.xml and disable plugins by
  setting <enable_plugins>0</enable_plugins>
- sudo koha-mysql kohadev -e "UPDATE systempreferences SET value='1' WHERE variable='ILLModule';"
- Run restart_all.
- In staff interface, go to About koha -> System information. The page
  should load normally (as opposed to returning 500 internal server
  error).

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 8 Pedro Amorim 2024-10-29 11:24:09 UTC
Created attachment 173619 [details] [review]
Bug 37872: Consider enable_plugins = 0 when fetching plugin backends

This patch builds on top of Andreas patch checking for Koha::Plugins->new() instead as the enable_plugins is already done there.
It also adds a check in reply to Joubu's observation, which is indeed an additional bug.

Test plan:
1) Apply tests patch. Run the tests. Notice it fails
2) Apply the remaining patches. Run the tests. Notice they pass.

Additional (and better) tests can be written, but for now I think this is good enough.
Additional tests should be written on top of bug 36197 (which adds a dummy ILL backend plugin for testing), at:
https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=36197&attachment=171371

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 9 Pedro Amorim 2024-10-29 11:26:48 UTC
Hi guys, I followed up on this a bit. @Andreas, @David are you available to take a look and let me know if you agree / sign-off?
Comment 10 Pedro Amorim 2024-10-30 10:15:49 UTC
Updating this to major as this may lead to a halt in service in the certain circumstances.
Comment 11 David Nind 2024-10-30 16:59:34 UTC
Created attachment 173727 [details] [review]
Bug 37872: Add tests

$ prove t/db_dependent/Koha/ILL/Request.t
$ prove t/db_dependent/Koha/ILL/Request/Config.t

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2024-10-30 16:59:37 UTC
Created attachment 173728 [details] [review]
Bug 37872: Add guards for enable_plugins

There are ILL-backends that are not implemented as
Koha plugins 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.

Testplan for koha-testing-docker:

- Edit /etc/koha/sites/kohadev/koha-conf.xml and disable plugins by
  setting <enable_plugins>0</enable_plugins>
- sudo koha-mysql kohadev -e "UPDATE systempreferences SET value='1' WHERE variable='ILLModule';"
- Run restart_all.
- In staff interface, go to About koha -> System information. The page
  should load normally (as opposed to returning 500 internal server
  error).

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2024-10-30 16:59:40 UTC
Created attachment 173729 [details] [review]
Bug 37872: Consider enable_plugins = 0 when fetching plugin backends

This patch builds on top of Andreas patch checking for Koha::Plugins->new() instead as the enable_plugins is already done there.
It also adds a check in reply to Joubu's observation, which is indeed an additional bug.

Test plan:
1) Apply tests patch. Run the tests. Notice it fails
2) Apply the remaining patches. Run the tests. Notice they pass.

Additional (and better) tests can be written, but for now I think this is good enough.
Additional tests should be written on top of bug 36197 (which adds a dummy ILL backend plugin for testing), at:
https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=36197&attachment=171371

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Comment 14 Martin Renvoize (ashimema) 2024-10-31 11:29:32 UTC
Created attachment 173771 [details] [review]
Bug 37872: Add tests

$ prove t/db_dependent/Koha/ILL/Request.t
$ prove t/db_dependent/Koha/ILL/Request/Config.t

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize (ashimema) 2024-10-31 11:29:34 UTC
Created attachment 173772 [details] [review]
Bug 37872: Add guards for enable_plugins

There are ILL-backends that are not implemented as
Koha plugins 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.

Testplan for koha-testing-docker:

- Edit /etc/koha/sites/kohadev/koha-conf.xml and disable plugins by
  setting <enable_plugins>0</enable_plugins>
- sudo koha-mysql kohadev -e "UPDATE systempreferences SET value='1' WHERE variable='ILLModule';"
- Run restart_all.
- In staff interface, go to About koha -> System information. The page
  should load normally (as opposed to returning 500 internal server
  error).

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize (ashimema) 2024-10-31 11:29:37 UTC
Created attachment 173773 [details] [review]
Bug 37872: Consider enable_plugins = 0 when fetching plugin backends

This patch builds on top of Andreas patch checking for Koha::Plugins->new() instead as the enable_plugins is already done there.
It also adds a check in reply to Joubu's observation, which is indeed an additional bug.

Test plan:
1) Apply tests patch. Run the tests. Notice it fails
2) Apply the remaining patches. Run the tests. Notice they pass.

Additional (and better) tests can be written, but for now I think this is good enough.
Additional tests should be written on top of bug 36197 (which adds a dummy ILL backend plugin for testing), at:
https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=36197&attachment=171371

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize (ashimema) 2024-10-31 11:30:06 UTC
I did a final run-through on this to get it over the line.. happy to call it PQA given all the eyes on it and my own testing.
Comment 18 Katrin Fischer 2024-11-07 13:25:03 UTC
Unit tests, updated POD and release notes! :)
Comment 19 Katrin Fischer 2024-11-07 15:32:49 UTC
Pushed for 24.11!

Well done everyone, thank you!