Bug 29121 - Plugins with broken ->install prevent access to the plugins list
Summary: Plugins with broken ->install prevent access to the plugins list
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-27 11:16 UTC by Tomás Cohen Arazi
Modified: 2021-10-08 13:51 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 29121: Add new plugin exceptions (3.96 KB, patch)
2021-09-27 11:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29121: Regression tests (6.08 KB, patch)
2021-09-27 11:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29121: Catch errors in ->install and ->upgrade calls on plugins (6.20 KB, patch)
2021-09-27 11:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29121: Add new plugin exceptions (4.02 KB, patch)
2021-09-27 11:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29121: Add new plugin exceptions (4.02 KB, patch)
2021-09-27 11:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29121: Regression tests (6.14 KB, patch)
2021-09-27 11:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29121: Catch errors in ->install and ->upgrade calls on plugins (6.25 KB, patch)
2021-09-27 11:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29121: Add POD to plugins (1.54 KB, patch)
2021-09-27 11:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29121: Add new plugin exceptions (4.07 KB, patch)
2021-09-30 09:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29121: Regression tests (6.20 KB, patch)
2021-09-30 09:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29121: Catch errors in ->install and ->upgrade calls on plugins (6.31 KB, patch)
2021-09-30 09:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29121: Add POD to plugins (1.60 KB, patch)
2021-09-30 09:59 UTC, Martin Renvoize
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 2021-09-27 11:16:56 UTC
If a plugin is badly coded, like adding a table in their install method, without checking it doesn't exist first, the plugins home page gives a 500 error.

To reproduce:
1. Install any plugin you like
2. Restart plack
=> SUCCESS: All good
3. Manually change its install method to:

sub install {
    die "plugin, die!";
}

4. Run:
   $ koha-mysql kohadev
   > DELETE FROM plugin_data;
(to make sure there's no __INSTALLED__ entry, do on a safe to delete DB).
5. Point your browser to the plugins-home.pl page
=> FAIL: Boom
Comment 1 Tomás Cohen Arazi 2021-09-27 11:40:09 UTC
Created attachment 125352 [details] [review]
Bug 29121: Add new plugin exceptions

This patch introduces two new exceptions:

- Koha::Exceptions::Plugin::InstallDied
- Koha::Exceptions::Plugin::UpgradeDied

Tests are added for their stringification output.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-09-27 11:40:13 UTC
Created attachment 125353 [details] [review]
Bug 29121: Regression tests

This patch adds regression tests for broken ->install and ->upgrade
methods on plugins.

It adds two dummie plugins named BrokenInstall and BrokenUpgrade, for
convenience.

Tests are added to catch the warnings that will be logged when trying to
load this plugins on different scenarios.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-09-27 11:40:17 UTC
Created attachment 125354 [details] [review]
Bug 29121: Catch errors in ->install and ->upgrade calls on plugins

This patch adds a try/catch block when instantiating plugins. Calling
->new on a plugin eventually triggers a call to ->install (this has
always been like this since bug 7804). If the ->install method is
somehow borked, then the process dies. We need to prevent that, and
report back some error took place. That's what this patch does.

The same happens to the ->upgrade.

To test:
1. Install any plugin you like
2. Restart plack (just in case)
=> SUCCESS: All good
3. Manually change its install method to:

sub install {
    die "plugin, die!";
}

4. Run:
   $ koha-mysql kohadev
   > DELETE FROM plugin_data;
(to make sure there's no __INSTALLED__ entry, do on a safe to delete DB).
5. Point your browser to the plugins-home.pl page
=> FAIL: Boom
6. Apply up to the regression tests
7. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Plugins/Plugins.t \
           t/Koha/Exceptions.t
=> FAIL: Tests fail!
8. Apply this patch
9. Repeat 2
=> SUCCESS: Tests pass!
10. Run:
    $ restart_all
11. Repeat 5
=> SUCCESS: The page is not broken
12. Sign off :-D

Note: I used
    $ kshell
   k$ perl misc/devel/install_plugins.pl
to test as well.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Kyle M Hall 2021-09-27 11:57:49 UTC
Created attachment 125357 [details] [review]
Bug 29121: Add new plugin exceptions

This patch introduces two new exceptions:

- Koha::Exceptions::Plugin::InstallDied
- Koha::Exceptions::Plugin::UpgradeDied

Tests are added for their stringification output.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Kyle M Hall 2021-09-27 11:58:26 UTC
Created attachment 125358 [details] [review]
Bug 29121: Add new plugin exceptions

This patch introduces two new exceptions:

- Koha::Exceptions::Plugin::InstallDied
- Koha::Exceptions::Plugin::UpgradeDied

Tests are added for their stringification output.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2021-09-27 11:58:36 UTC
Created attachment 125359 [details] [review]
Bug 29121: Regression tests

This patch adds regression tests for broken ->install and ->upgrade
methods on plugins.

It adds two dummie plugins named BrokenInstall and BrokenUpgrade, for
convenience.

Tests are added to catch the warnings that will be logged when trying to
load this plugins on different scenarios.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2021-09-27 11:58:39 UTC
Created attachment 125360 [details] [review]
Bug 29121: Catch errors in ->install and ->upgrade calls on plugins

This patch adds a try/catch block when instantiating plugins. Calling
->new on a plugin eventually triggers a call to ->install (this has
always been like this since bug 7804). If the ->install method is
somehow borked, then the process dies. We need to prevent that, and
report back some error took place. That's what this patch does.

The same happens to the ->upgrade.

To test:
1. Install any plugin you like
2. Restart plack (just in case)
=> SUCCESS: All good
3. Manually change its install method to:

sub install {
    die "plugin, die!";
}

4. Run:
   $ koha-mysql kohadev
   > DELETE FROM plugin_data;
(to make sure there's no __INSTALLED__ entry, do on a safe to delete DB).
5. Point your browser to the plugins-home.pl page
=> FAIL: Boom
6. Apply up to the regression tests
7. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Plugins/Plugins.t \
           t/Koha/Exceptions.t
=> FAIL: Tests fail!
8. Apply this patch
9. Repeat 2
=> SUCCESS: Tests pass!
10. Run:
    $ restart_all
11. Repeat 5
=> SUCCESS: The page is not broken
12. Sign off :-D

Note: I used
    $ kshell
   k$ perl misc/devel/install_plugins.pl
to test as well.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2021-09-27 11:58:42 UTC
Created attachment 125361 [details] [review]
Bug 29121: Add POD to plugins

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Martin Renvoize 2021-09-30 09:59:23 UTC
Created attachment 125477 [details] [review]
Bug 29121: Add new plugin exceptions

This patch introduces two new exceptions:

- Koha::Exceptions::Plugin::InstallDied
- Koha::Exceptions::Plugin::UpgradeDied

Tests are added for their stringification output.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2021-09-30 09:59:27 UTC
Created attachment 125478 [details] [review]
Bug 29121: Regression tests

This patch adds regression tests for broken ->install and ->upgrade
methods on plugins.

It adds two dummie plugins named BrokenInstall and BrokenUpgrade, for
convenience.

Tests are added to catch the warnings that will be logged when trying to
load this plugins on different scenarios.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2021-09-30 09:59:31 UTC
Created attachment 125479 [details] [review]
Bug 29121: Catch errors in ->install and ->upgrade calls on plugins

This patch adds a try/catch block when instantiating plugins. Calling
->new on a plugin eventually triggers a call to ->install (this has
always been like this since bug 7804). If the ->install method is
somehow borked, then the process dies. We need to prevent that, and
report back some error took place. That's what this patch does.

The same happens to the ->upgrade.

To test:
1. Install any plugin you like
2. Restart plack (just in case)
=> SUCCESS: All good
3. Manually change its install method to:

sub install {
    die "plugin, die!";
}

4. Run:
   $ koha-mysql kohadev
   > DELETE FROM plugin_data;
(to make sure there's no __INSTALLED__ entry, do on a safe to delete DB).
5. Point your browser to the plugins-home.pl page
=> FAIL: Boom
6. Apply up to the regression tests
7. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Plugins/Plugins.t \
           t/Koha/Exceptions.t
=> FAIL: Tests fail!
8. Apply this patch
9. Repeat 2
=> SUCCESS: Tests pass!
10. Run:
    $ restart_all
11. Repeat 5
=> SUCCESS: The page is not broken
12. Sign off :-D

Note: I used
    $ kshell
   k$ perl misc/devel/install_plugins.pl
to test as well.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2021-09-30 09:59:34 UTC
Created attachment 125480 [details] [review]
Bug 29121: Add POD to plugins

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2021-09-30 09:59:57 UTC
Works as expected, code is clear and passes qa scripts

Passing QA
Comment 14 Jonathan Druart 2021-10-01 14:30:19 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 15 Kyle M Hall 2021-10-08 13:51:14 UTC
Can you please provide a rebased patch set for 21.05.x?

Thanks!