Bug 29826 - Manage call of Template Plugin Branches GetName() with null or empty branchcode
Summary: Manage call of Template Plugin Branches GetName() with null or empty branchcode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on: 26587
Blocks:
  Show dependency treegraph
 
Reported: 2022-01-08 01:33 UTC by Fridolin Somers
Modified: 2022-12-12 21:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.04


Attachments
Bug 29826: Remove warning in t/db_dependent/Template/Plugin/Branches.t (1.23 KB, patch)
2022-01-08 01:37 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29826: Remove warning in t/db_dependent/Template/Plugin/Branches.t (1.28 KB, patch)
2022-01-19 23:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 29826: Manage call of Template Plugin Branches GetName() with null or empty branchcode (2.29 KB, patch)
2022-02-01 00:23 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29826: Manage call of Template Plugin Branches GetName() with null or empty branchcode (2.33 KB, patch)
2022-02-06 06:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 29826: Manage call of Template Plugin Branches GetName() with null or empty branchcode (2.40 KB, patch)
2022-03-01 14: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 Fridolin Somers 2022-01-08 01:33:48 UTC
Bug 26587 added a concatenation that sends a warning if var is undef :
$ prove t/db_dependent/Template/Plugin/Branches.t
Use of uninitialized value $branchcode in concatenation (.) or string at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 35.
Comment 1 Fridolin Somers 2022-01-08 01:37:25 UTC
Created attachment 129208 [details] [review]
Bug 29826: Remove warning in t/db_dependent/Template/Plugin/Branches.t

Bug 26587 added a concatenation that sends a warning if var is undef :
$ prove t/db_dependent/Template/Plugin/Branches.t
Use of uninitialized value $branchcode in concatenation (.) or string at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 35.

This patch adds an early return empty string when GetName is called
without $branchcode

Test plan :
1) Run t/db_dependent/Template/Plugin/Branches.t without patch
2) Run with the patch to see warning disappear
Comment 2 David Nind 2022-01-19 23:26:45 UTC
Created attachment 129630 [details] [review]
Bug 29826: Remove warning in t/db_dependent/Template/Plugin/Branches.t

Bug 26587 added a concatenation that sends a warning if var is undef :
$ prove t/db_dependent/Template/Plugin/Branches.t
Use of uninitialized value $branchcode in concatenation (.) or string at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 35.

This patch adds an early return empty string when GetName is called
without $branchcode

Test plan :
1) Run t/db_dependent/Template/Plugin/Branches.t without patch
2) Run with the patch to see warning disappear

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Jonathan Druart 2022-01-27 15:32:19 UTC
I don't know if this is correct. We should never call the plugin without parameter. In my opinion the test is wrong and should be removed.
Comment 4 Fridolin Somers 2022-01-28 06:26:31 UTC
(In reply to Jonathan Druart from comment #3)
> I don't know if this is correct. We should never call the plugin without
> parameter. In my opinion the test is wrong and should be removed.

This may occur with suggestions that can have branch undefined.
Comment 5 Marcel de Rooy 2022-01-28 10:15:38 UTC
This report refers to the test. So the suggestion thing might be a different case. Or widen the scope by changing the title?

+    return q{} unless defined $branchcode;
This still has the effect of testing if there is a branch with an empty code. Which makes no sense either?
So you could also return unless $branchcode.
Comment 6 Fridolin Somers 2022-02-01 00:06:04 UTC
(In reply to Marcel de Rooy from comment #5)
> This report refers to the test. So the suggestion thing might be a different
> case. Or widen the scope by changing the title?
Ah good point, I change title.

> 
> +    return q{} unless defined $branchcode;
> This still has the effect of testing if there is a branch with an empty
> code. Which makes no sense either?
> So you could also return unless $branchcode.
Ah indeed.
But we must allow branchcode=0.
Comment 7 Fridolin Somers 2022-02-01 00:23:57 UTC
Created attachment 130030 [details] [review]
Bug 29826: Manage call of Template Plugin Branches GetName() with null or empty branchcode

Bug 26587 added a concatenation that sends a warning if var is undef :
$ prove t/db_dependent/Template/Plugin/Branches.t
Use of uninitialized value $branchcode in concatenation (.) or string at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 35.

This patch adds an early return empty string when GetName is called with $branchcode null or empty string.

Test plan :
1) Run t/db_dependent/Template/Plugin/Branches.t without patch
2) Run with the patch to see warning disappear
Comment 8 David Nind 2022-02-06 06:15:10 UTC
Comment on attachment 129630 [details] [review]
Bug 29826: Remove warning in t/db_dependent/Template/Plugin/Branches.t

Replaced by later patch
Comment 9 David Nind 2022-02-06 06:18:34 UTC
Created attachment 130201 [details] [review]
Bug 29826: Manage call of Template Plugin Branches GetName() with null or empty branchcode

Bug 26587 added a concatenation that sends a warning if var is undef :
$ prove t/db_dependent/Template/Plugin/Branches.t
Use of uninitialized value $branchcode in concatenation (.) or string at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 35.

This patch adds an early return empty string when GetName is called with $branchcode null or empty string.

Test plan :
1) Run t/db_dependent/Template/Plugin/Branches.t without patch
2) Run with the patch to see warning disappear

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Martin Renvoize 2022-03-01 14:59:01 UTC
Created attachment 131183 [details] [review]
Bug 29826: Manage call of Template Plugin Branches GetName() with null or empty branchcode

Bug 26587 added a concatenation that sends a warning if var is undef :
$ prove t/db_dependent/Template/Plugin/Branches.t
Use of uninitialized value $branchcode in concatenation (.) or string at /kohadevbox/koha/Koha/Template/Plugin/Branches.pm line 35.

This patch adds an early return empty string when GetName is called with $branchcode null or empty string.

Test plan :
1) Run t/db_dependent/Template/Plugin/Branches.t without patch
2) Run with the patch to see warning disappear

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2022-03-01 14:59:34 UTC
Great fix, includes tests, no regressions and squelched warnings.

Passing QA
Comment 12 Fridolin Somers 2022-03-04 02:12:04 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 13 Kyle M Hall 2022-03-04 14:59:57 UTC
Pushed to 21.11.x for 21.11.04
Comment 14 Andrew Fuerste-Henry 2022-03-20 15:54:03 UTC
Missing dependency, not backported to 21.05