37872 – ILL module has issues when plugins are disabled (enable_plugins = 0)

Bug 37872 - ILL module has issues when plugins are disabled (enable_plugins = 0)

Summary: ILL module has issues when plugins are disabled (enable_plugins = 0)

Status:	RESOLVED FIXED

Alias:	None

Product:	Koha
Classification:	Unclassified
Component:	Plugin architecture (show other bugs)
Version:	24.05
Hardware:	All All

Importance:	P3 major
Assignee:	Andreas Jonsson
QA Contact:	Pedro Amorim

URL:
Keywords:

Depends on:
Blocks:

Reported:	2024-09-09 18:13 UTC by Andreas Jonsson
Modified:	2025-01-02 09:15 UTC (History)
CC List:	7 users (show)

See Also:
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,24.05.06
Circulation function:

Attachments
Bug 37872: Add guards for enable_plugins (1.45 KB, patch) 2024-09-09 18:15 UTC, Andreas Jonsson	Details \| Diff \| Splinter Review
Bug 37872: Add guards for enable_plugins (1.50 KB, patch) 2024-09-09 21:46 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 37872: Add tests (2.44 KB, patch) 2024-10-29 11:24 UTC, Pedro Amorim	Details \| Diff \| Splinter Review
Bug 37872: Add guards for enable_plugins (1.55 KB, patch) 2024-10-29 11:24 UTC, Pedro Amorim	Details \| Diff \| Splinter Review
Bug 37872: Consider enable_plugins = 0 when fetching plugin backends (2.74 KB, patch) 2024-10-29 11:24 UTC, Pedro Amorim	Details \| Diff \| Splinter Review
Bug 37872: Add tests (2.48 KB, patch) 2024-10-30 16:59 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 37872: Add guards for enable_plugins (1.60 KB, patch) 2024-10-30 16:59 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 37872: Consider enable_plugins = 0 when fetching plugin backends (2.79 KB, patch) 2024-10-30 16:59 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 37872: Add tests (2.55 KB, patch) 2024-10-31 11:29 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Bug 37872: Add guards for enable_plugins (1.67 KB, patch) 2024-10-31 11:29 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Bug 37872: Consider enable_plugins = 0 when fetching plugin backends (2.85 KB, patch) 2024-10-31 11:29 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

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!

Comment 20 Lucas Gass (lukeg) 2024-12-05 21:35:26 UTC

Backported to 24.05.x for upcoming 24.05.06

Comment 21 Fridolin Somers 2024-12-18 09:52:08 UTC

Does not apply easy on 23.11.x
Looks like depends on Bug 35581 for test suite
Can I backport only main patch ?

Comment 22 Pedro Amorim 2024-12-18 09:54:56 UTC

(In reply to Fridolin Somers from comment #21)
> Does not apply easy on 23.11.x
> Looks like depends on Bug 35581 for test suite
> Can I backport only main patch ?

Hi Frido, I'm fairly confident this is only required after bug 19605, and that was only pushed to 24.05. So no further backport should be required here.

Comment 23 Pedro Amorim 2024-12-18 09:57:30 UTC

(In reply to Pedro Amorim from comment #22)
> Hi Frido, I'm fairly confident this is only required after bug 19605, and
> that was only pushed to 24.05. So no further backport should be required
> here.

Upon second read, I may be wrong here, it may have been originally introduced by bug 30719 (23.11).
Let me test this on 23.11.x, confirm the bug is there and see if I can provide a rebased version of main patch.

Comment 24 Pedro Amorim 2024-12-18 10:26:44 UTC

(In reply to Pedro Amorim from comment #23)
> Upon second read, I may be wrong here, it may have been originally
> introduced by bug 30719 (23.11).
> Let me test this on 23.11.x, confirm the bug is there and see if I can
> provide a rebased version of main patch.

Tested this on 23.11.x, there's no issue. This was indeed caused by bug 19605 not in 23.11.x. No backport needed.

Comment 25 Fridolin Somers 2024-12-23 13:55:09 UTC

Thanks a lot for your tests Pedro

Comment 26 Aude Charillon 2025-01-02 09:15:21 UTC

No need for any updates to the Koha Manual.