| Summary: | ILL module has issues when plugins are disabled (enable_plugins = 0) | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Andreas Jonsson <andreas.jonsson> |
| Component: | Plugin architecture | Assignee: | Andreas Jonsson <andreas.jonsson> |
| Status: | Pushed to oldstable --- | QA Contact: | Pedro Amorim <pedro.amorim> |
| Severity: | major | ||
| Priority: | P3 | CC: | david, fridolin.somers, jonathan.druart, lucas, 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,24.05.06
|
| 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
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). 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> Testing notes (using KTD): 1. I tested on main. 2. I updated the assignee 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?
Looking here 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> 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> 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> 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? Updating this to major as this may lead to a halt in service in the certain circumstances. 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> 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> 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> 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> 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> 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> 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. Unit tests, updated POD and release notes! :) Pushed for 24.11! Well done everyone, thank you! Backported to 24.05.x for upcoming 24.05.06 Does not apply easy on 23.11.x Looks like depends on Bug 35581 for test suite Can I backport only main patch ? (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. (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. (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. |