Bug 36419

Summary: Ambiguous return value in GetPlugins
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, kyle, m.de.rooy, martin.renvoize, nick, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 35536    
Bug Blocks:    
Attachments: [POC] Bug 36419: Use wantarray to control return value
Bug 36419: Use wantarray to control return value

Description Tomás Cohen Arazi (tcohen) 2024-03-25 17:01:32 UTC
Bug 35536 introduced some better error handling, but made the return value more erratic.

We you previously had code like this:

```perl
    my @plugins = Koha::Plugins->new()->GetPlugins(
        {
            method   => 'ill_backend',
            metadata => { name => $backend_id },
            all      => 1,
            errors   => 1
        }
    );

    foreach my $plugin (@plugins) { ... }
```

And now the new return value for GetPlugins is:

```perl
    return $errors ? ( \@plugins, \@failing ) : @plugins;
```

This means the caller will need to check the return value, and if the arity is two (?), check that the second element has some specific structure so we determine... it is not actually a plugin!

I propose we return a hashref with the plugins and (possibly) the errors.
Comment 1 Tomás Cohen Arazi (tcohen) 2024-03-25 17:55:14 UTC
I have now noticed that the `errors => 1` from the original code was the one preventing it to work.

So I'll lower the importance to enhancement, but want to propose a signature change here. For discussion.
Comment 2 Tomás Cohen Arazi (tcohen) 2024-03-25 18:07:49 UTC
Created attachment 163850 [details] [review]
[POC] Bug 36419: Use wantarray to control return value

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Kyle M Hall (khall) 2024-03-26 10:36:41 UTC
Created attachment 163895 [details] [review]
Bug 36419: Use wantarray to control return value

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Kyle M Hall (khall) 2024-03-26 10:37:22 UTC
Looks good to me!
Comment 5 Marcel de Rooy 2024-03-26 12:15:56 UTC
Idea is fine with me, current implementation would fail.
Should we only return this good, bad (and ugly) if the errors param was passed?
Otherwise we will need to examine all calls of GetPlugins without errors => 1 too.

Plugins.t fails on a call now with errors, expecting the old returns.
Comment 6 Kyle M Hall (khall) 2024-03-26 13:07:13 UTC
(In reply to Marcel de Rooy from comment #5)
> Idea is fine with me, current implementation would fail.
> Should we only return this good, bad (and ugly) if the errors param was
> passed?
> Otherwise we will need to examine all calls of GetPlugins without errors =>
> 1 too.
> 
> Plugins.t fails on a call now with errors, expecting the old returns.

Correct! I believe this patch is just a POC and not expected to fully function.