Bug 28211

Summary: Replace use of call_recursive() with call()
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Plugin architectureAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, jonathan.druart, julian.maurice, m.de.rooy, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement changes the way plugin hooks are called to transform data. We now pass object to be modified as a reference, thus allowing several plugins to operate cumulatively on the same object.
Version(s) released in:
21.11.00
Bug Depends on: 26352    
Bug Blocks:    
Attachments: Bug 28211: Replace use of call_recursive() with call()
Bug 28211 Fix count of plugins that should be installed for unit test
Bug 28211: Replace use of call_recursive() with call()
Bug 28211 Fix count of plugins that should be installed for unit test
Bug 28211: Replace use of call_recursive() with call()
Bug 28211: Replace use of call_recursive() with call()
Bug 28211: Fix count of plugins that should be installed for unit test
Bug 28211: Replace use of call_recursive() with call()
Bug 28211: Replace use of call_recursive() with call()
Bug 28211: (QA follow-up) Remove test_call_recursive
Bug 28211: (QA follow-up) POD changes
Bug 28211: (QA follow-up) Add unit tests
Bug 28211: Fix test
Bug 28211: (QA follow-up) Test for hook calls properly

Description Kyle M Hall 2021-04-23 15:53:30 UTC
This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call.
Comment 1 Kyle M Hall 2021-04-23 15:59:40 UTC
Created attachment 120120 [details] [review]
Bug 28211: Replace use of call_recursive() with call()

This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Plugins/Plugins.t
3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t
Comment 2 Andrew Fuerste-Henry 2021-05-12 12:14:50 UTC
I get this test failure:
root@kohadevbox:koha(master)$ prove t/db_dependent/Koha/Plugins/Plugins.t
t/db_dependent/Koha/Plugins/Plugins.t .. 5/55 Plugin error (Test Plugin): item_barcode_transform called with parameter: 1 at /kohadevbox/koha/Koha/Plugins.pm line 74.
Plugin error (Test Plugin): item_barcode_transform called with parameter: abcd at /kohadevbox/koha/Koha/Plugins.pm line 74.

    #   Failed test 'Only four plugins found via a metadata tag'
    #   at t/db_dependent/Koha/Plugins/Plugins.t line 129.
    #          got: '2'
    #     expected: '4'
    # Looks like you failed 1 test of 2.

#   Failed test 'GetPlugins() tests'
#   at t/db_dependent/Koha/Plugins/Plugins.t line 132.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 279.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 280.
Use of uninitialized value $interface in string eq at /kohadevbox/koha/C4/Templates.pm line 282.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
Use of uninitialized value $lang in concatenation (.) or string at /kohadevbox/koha/C4/Templates.pm line 296.
t/db_dependent/Koha/Plugins/Plugins.t .. 50/55 # Looks like you failed 1 test of 55.
t/db_dependent/Koha/Plugins/Plugins.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/55 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Plugins/Plugins.t (Wstat: 256 Tests: 55 Failed: 1)
  Failed test:  7
  Non-zero exit status: 1
Files=1, Tests=55,  5 wallclock secs ( 0.02 usr  0.01 sys +  1.80 cusr  0.39 csys =  2.22 CPU)
Result: FAIL
Comment 3 Kyle M Hall 2021-05-12 13:42:22 UTC
Created attachment 120875 [details] [review]
Bug 28211 Fix count of plugins that should be installed for unit test
Comment 4 Andrew Fuerste-Henry 2021-05-12 13:53:50 UTC
Created attachment 120882 [details] [review]
Bug 28211: Replace use of call_recursive() with call()

This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Plugins/Plugins.t
3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 5 Andrew Fuerste-Henry 2021-05-12 13:53:54 UTC
Created attachment 120883 [details] [review]
Bug 28211 Fix count of plugins that should be installed for unit test

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 6 Marcel de Rooy 2021-06-23 14:13:33 UTC
I would recommend to merge this report with the call_recursive report. Both are signed off. We could eliminate the code we actually do not want to push.

The call_recursive name was a bit misleading too btw.
Comment 7 Kyle M Hall 2021-06-23 15:30:16 UTC
(In reply to Marcel de Rooy from comment #6)
> I would recommend to merge this report with the call_recursive report. Both
> are signed off. We could eliminate the code we actually do not want to push.
> 
> The call_recursive name was a bit misleading too btw.

The two bugs are on the other sides of 26351 and 26352. Merging them would require modifying the patches for those bugs, both of which have passed qa. I think we should simply push the four bugs as a unit rather than risk introducing regressions.
Comment 8 Marcel de Rooy 2021-06-24 06:02:59 UTC
(In reply to Kyle M Hall from comment #7)
> (In reply to Marcel de Rooy from comment #6)
> > I would recommend to merge this report with the call_recursive report. Both
> > are signed off. We could eliminate the code we actually do not want to push.
> > 
> > The call_recursive name was a bit misleading too btw.
> 
> The two bugs are on the other sides of 26351 and 26352. Merging them would
> require modifying the patches for those bugs, both of which have passed qa.
> I think we should simply push the four bugs as a unit rather than risk
> introducing regressions.

Okay. I missed the other two ;)
Comment 9 Marcel de Rooy 2021-06-25 08:55:10 UTC
Applying: Bug 26351: Clean up includes in Circulation.pm
error: sha1 information is lacking or useless (C4/Circulation.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 26351: Clean up includes in Circulation.pm
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-26351-Clean-up-includes-in-Circulationpm-DpTlZT.patch

Cannot apply cleanly patches from bug 26351. Everything will be left dirty.
git bz apply --continue will not continue the process if patches from other bug reports need to be applied.
Comment 10 Kyle M Hall 2021-06-25 11:59:12 UTC
Patches applied fine for me. Did you apply on top of 26352?
Comment 11 Kyle M Hall 2021-06-25 12:00:10 UTC
Created attachment 122425 [details] [review]
Bug 28211: Replace use of call_recursive() with call()

This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Plugins/Plugins.t
3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 12 Kyle M Hall 2021-06-25 12:01:02 UTC
Created attachment 122426 [details] [review]
Bug 28211: Replace use of call_recursive() with call()

This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Plugins/Plugins.t
3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 13 Kyle M Hall 2021-06-25 12:01:44 UTC
Created attachment 122427 [details] [review]
Bug 28211: Fix count of plugins that should be installed for unit test

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 14 Marcel de Rooy 2021-10-01 09:18:06 UTC
Asked a question about one of the base reports still not pushed.
Comment 15 Kyle M Hall 2021-10-07 14:58:21 UTC
Created attachment 125892 [details] [review]
Bug 28211: Replace use of call_recursive() with call()

This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Plugins/Plugins.t
3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t
Comment 16 Marcel de Rooy 2021-10-08 06:41:57 UTC
QAing
Comment 17 Marcel de Rooy 2021-10-08 07:13:46 UTC
Created attachment 125926 [details] [review]
Bug 28211: Replace use of call_recursive() with call()

This is based on Julian's idea on bug 28026 where we could get rid of call_recursive by passing refs as arguments to call.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Koha/Plugins/Plugins.t
3) prove t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2021-10-08 07:13:49 UTC
Created attachment 125927 [details] [review]
Bug 28211: (QA follow-up) Remove test_call_recursive

Obsoleted now.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2021-10-08 07:13:53 UTC
Created attachment 125928 [details] [review]
Bug 28211: (QA follow-up) POD changes

[1] Resolving qa warn on POD coverage.
[2] Adding a line about the passing refs-tric.
[3] Removing an outdated line on available methods.
[4] Removing POD section for after_hold_create. Strayed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2021-10-08 07:15:19 UTC
No blocker. But this behavior seems a little bit hacky to me. I would personally rather get back a value from a plugin and decide what to do with it.
Comment 21 Jonathan Druart 2021-10-14 10:30:48 UTC
I think we are missing a test (passing refs from one plugin to another) for ->call, can you double check please?
Comment 22 Kyle M Hall 2021-10-14 11:58:13 UTC
Created attachment 126274 [details] [review]
Bug 28211: (QA follow-up) Add unit tests
Comment 23 Jonathan Druart 2021-10-18 09:33:54 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 24 Jonathan Druart 2021-10-18 09:39:43 UTC
We should highlight this behaviour in the release notes and on the wiki page.
Comment 25 Jonathan Druart 2021-10-18 11:06:44 UTC
Tests are failing, can you fix ASAP please?

t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 1/4 Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
    #   Failed test 'Plugin enabled, route defined'
    #   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 125.
    # Looks like you failed 1 test of 2.t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 2/4 
#   Failed test 'Disabled plugins tests'#   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 129.
Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
Plugin error (Test Plugin): patron_barcode_transform called with parameter: swJd8T4G9NdzlYsedEoNpzbbda70 at /kohadevbox/koha/Koha/Plugins.pm line 84.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 3/4 Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'install' died for plugin Koha::Plugin::BrokenInstall at /kohadevbox/koha/Koha/Plugins.pm line 199.
Calling 'upgrade' died for plugin Koha::Plugin::BrokenUpgrade at /kohadevbox/koha/Koha/Plugins.pm line 199.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 4/4 # Looks like you failed 1 test of 4.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t (Wstat: 256 Tests: 4 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=4, 31 wallclock secs ( 0.03 usr  0.00 sys + 30.24 cusr  0.41 csys = 30.68 CPU)
Result: FAIL
Comment 26 Kyle M Hall 2021-10-18 13:35:13 UTC
(In reply to Jonathan Druart from comment #25)
> Tests are failing, can you fix ASAP please?

I'm not seeing any reason this test module is causing the failures. If you rm that test module, the unit tests pass! Tomas is going to take a look to see if he can spot something I missed.
Comment 27 Jonathan Druart 2021-10-19 07:26:38 UTC
Created attachment 126471 [details] [review]
Bug 28211: Fix test

#   Failed test 'Plugin enabled, route defined'
    #   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 125.
    # Looks like you failed 1 test of 2.

105         $good_plugin = $plugin
106             if $plugin->{metadata}->{description} eq 'Test plugin';

The wrong plugin was considered the good one.
Comment 28 Jonathan Druart 2021-10-19 07:45:59 UTC
Patch pushed to master.
Comment 29 Tomás Cohen Arazi 2021-10-19 12:49:31 UTC
Created attachment 126497 [details] [review]
Bug 28211: (QA follow-up) Test for hook calls properly

Some warnings, that mean a hook has been called were not tested.
This patch adds those tests, and does some tidy also.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Jonathan Druart 2021-10-19 13:31:45 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> Created attachment 126497 [details] [review] [review]
> Bug 28211: (QA follow-up) Test for hook calls properly
> 
> Some warnings, that mean a hook has been called were not tested.
> This patch adds those tests, and does some tidy also.
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Pushed to master!