Bug 36419 - Ambiguous return value in GetPlugins
Summary: Ambiguous return value in GetPlugins
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on: 35536
Blocks:
  Show dependency treegraph
 
Reported: 2024-03-25 17:01 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2024-03-26 13:07 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
[POC] Bug 36419: Use wantarray to control return value (2.51 KB, patch)
2024-03-25 18:07 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 36419: Use wantarray to control return value (2.55 KB, patch)
2024-03-26 10:36 UTC, Kyle M Hall (khall)
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 (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.