Bug 15135 - Remove Warning Subroutine HasOverdues redefined
Summary: Remove Warning Subroutine HasOverdues redefined
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marc Véron
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-05 10:06 UTC by Marc Véron
Modified: 2017-06-14 22:07 UTC (History)
8 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:


Attachments
Bug 15135: Remove Warning Subroutine HasOverdues redefined (1.17 KB, patch)
2015-11-05 10:14 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15135: Remove Warning Subroutine HasOverdues redefined (1.23 KB, patch)
2015-11-06 06:50 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 15135 [QA Followup] - Switch from use to require (711 bytes, patch)
2015-11-25 18:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15135: Get rid of Warning 'Subroutine HasOverdues redefined' (2.34 KB, patch)
2015-12-30 13:49 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15135: Get rid of Warning 'Subroutine HasOverdues redefined' (2.39 KB, patch)
2016-01-01 02:44 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15135: Remove Warning Subroutine HasOverdues redefined (1.23 KB, patch)
2016-01-11 11:02 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15135: Remove Warning Subroutine HasOverdues redefined (1.38 KB, patch)
2016-01-11 11:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15135: Remove Warning Subroutine HasOverdues redefined (1.39 KB, patch)
2016-01-11 11:28 UTC, Jacek Ablewicz
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-11-05 10:06:33 UTC
In staff client, while displying patron related pages (e.g. moremember.pl) you get a warning like:
moremember.pl: Subroutine HasOverdues redefined at /usr/share/kohaclone/Koha/Template/Plugin/Borrowers.pm line 52.
This occurs with all pages that have [% USE Borrowers %] in their templates (directly or via members-toolbar.inc)

To reproduce:
- Go to Home > Patrons > Patron details for....
- Search for warning above in log
Comment 1 Marc Véron 2015-11-05 10:14:14 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2015-11-06 06:50:31 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-11-06 11:20:34 UTC
The plugin use a C4::Members subroutine, you should not remove this line.
Comment 4 Marc Véron 2015-11-06 11:49:27 UTC
(In reply to Jonathan Druart from comment #3)
> The plugin use a C4::Members subroutine, you should not remove this line.

Yes, you are right,there is a soubroutine called in line 57:
return C4::Members::HasOverdues($borrowernumber);

Because of the way it is called I supposed it is save to remove line 26:
 	
use C4::Members qw(HasOverdues);

My mistake if that is wrong. 

What would be an alternative to get rid of the warning?
Comment 5 Jonathan Druart 2015-11-06 14:42:20 UTC
(In reply to Marc Véron from comment #4)
> What would be an alternative to get rid of the warning?

Rename the subroutine or
  no warnings 'redefine';
But the no warnings is not a good solution.
Comment 6 Kyle M Hall 2015-11-25 18:03:46 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-12-02 10:31:19 UTC
Kyle, it's not a good idea to replace use with require, you are skipping the compilation pass.
Comment 8 Marc Véron 2015-12-30 13:49:10 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2015-12-30 13:53:11 UTC
New solution with renaming, see comment #5 and bug 12933 comment #67.

Switching back to NSO
Comment 10 Chris Cormack 2016-01-01 02:44:34 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2016-01-04 12:39:33 UTC
Marc,
The problem is that we have the same issue with IsDebarred.
The current code tricks us using not exporting the IsDebarred method:

  use Koha::Borrower::Debarments qw();

Either we continue to trick:

  -use C4::Members qw(HasOverdues);
  +use C4::Members qw();

Or we fix both.
But I am pretty sure that introducing another workaround is a good idea.

I would like to get Martin and Kyle POV here.
Comment 12 Jonathan Druart 2016-01-04 12:41:23 UTC
(In reply to Jonathan Druart from comment #11)

I should read before clicking the submit button...

> The current code tricks us using not exporting the IsDebarred method:

s/using//

> But I am pretty sure that introducing another workaround is a good idea.

is *not* a good idea
Comment 13 Marc Véron 2016-01-04 17:06:36 UTC
Sorry, I have no other ideas.
Maybe someone else finds a solution.
Comment 14 Martin Renvoize 2016-01-05 09:19:06 UTC
I agree adding another workaround is bad, doing it 'properly' is better.

So, am I reading it right that basically we also need IsDebarred also renaming and then this patch could stand.. or have I misunderstood something?
Comment 15 Jonathan Druart 2016-01-05 13:44:58 UTC
(In reply to Martin Renvoize from comment #14)
> I agree adding another workaround is bad, doing it 'properly' is better.
> 
> So, am I reading it right that basically we also need IsDebarred also
> renaming and then this patch could stand.. or have I misunderstood something?

No, you got it.
Renaming them could work, but otoh it would make sense to keep the same name: the subroutines only call the corresponding subroutine/method.
Comment 16 Marcel de Rooy 2016-01-11 09:13:53 UTC
If I understood correctly, the problem is that we now have Borrowers::HasOverdues and Members::HasOverdues and similar for IsDebarred.
Looking at the Borrowers plugin, not importing Overdues from Members there, resolves the warning for now and is consistent with the IsDebarred case. So I agree with Jonathan.

Marc: Could you adjust your patch?

Too bad btw that TT forces us to create a stub for each Koha routine we want to call in the templates. (Exception for OO modules as below)

From the TT docs:
===
If a regular Perl module supports an object-oriented interface and a new() constructor then it can be loaded and instantiated automatically like:
[% USE file = IO.File('/tmp/mydata') %]
===
Comment 17 Marc Véron 2016-01-11 10:00:52 UTC
(In reply to Marcel de Rooy from comment #16)
> If I understood correctly, the problem is that we now have
> Borrowers::HasOverdues and Members::HasOverdues and similar for IsDebarred.
> Looking at the Borrowers plugin, not importing Overdues from Members there,
> resolves the warning for now and is consistent with the IsDebarred case. So
> I agree with Jonathan.
> 
> Marc: Could you adjust your patch?
> 
> Too bad btw that TT forces us to create a stub for each Koha routine we want
> to call in the templates. (Exception for OO modules as below)
> 
> From the TT docs:
> ===
> If a regular Perl module supports an object-oriented interface and a new()
> constructor then it can be loaded and instantiated automatically like:
> [% USE file = IO.File('/tmp/mydata') %]
> ===

"...not importing Overdues from Members there..."
Sorry, I lost track here (Ik ben the kluts kwijt). Wasn't that what I did in my very first patch?
Comment 18 Marcel de Rooy 2016-01-11 10:39:27 UTC
(In reply to Marc Véron from comment #17)
> (In reply to Marcel de Rooy from comment #16)
> > If I understood correctly, the problem is that we now have
> > Borrowers::HasOverdues and Members::HasOverdues and similar for IsDebarred.
> > Looking at the Borrowers plugin, not importing Overdues from Members there,
> > resolves the warning for now and is consistent with the IsDebarred case. So
> > I agree with Jonathan.
> > 
> > Marc: Could you adjust your patch?
> > 
> > Too bad btw that TT forces us to create a stub for each Koha routine we want
> > to call in the templates. (Exception for OO modules as below)
> > 
> > From the TT docs:
> > ===
> > If a regular Perl module supports an object-oriented interface and a new()
> > constructor then it can be loaded and instantiated automatically like:
> > [% USE file = IO.File('/tmp/mydata') %]
> > ===
> 
> "...not importing Overdues from Members there..."
> Sorry, I lost track here (Ik ben the kluts kwijt). Wasn't that what I did in
> my very first patch?

No you removed the use completely there.
You only should say: use Module qw();
In that case you do not import any routines in the current namespace.
Comment 19 Marc Véron 2016-01-11 11:02:00 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2016-01-11 11:04:01 UTC
Not quit sure, should that go back to "Needs Signoff"?
Comment 21 Marcel de Rooy 2016-01-11 11:12:29 UTC
(In reply to Marc Véron from comment #20)
> Not quit sure, should that go back to "Needs Signoff"?

I will add a signoff
Comment 22 Marcel de Rooy 2016-01-11 11:16:04 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2016-01-11 11:17:17 UTC
This patch now received enough attention to pass QA.
Comment 24 Jacek Ablewicz 2016-01-11 11:28:57 UTC
Created attachment 46481 [details] [review]
Bug 15135: Remove Warning Subroutine HasOverdues redefined

In staff client, while displying patron related pages (e.g. moremember.pl) you get a warning like:
moremember.pl: Subroutine HasOverdues redefined at /usr/share/kohaclone/Koha/Template/Plugin/Borrowers.pm line 52.
This occurs with all pages that have [% USE Borrowers %] in their templates (directly or via members-toolbar.inc)

To test:
- Go to Home > Patrons > Patron details for....
- Search for warning above in log
- Apply patch
- Verify that no more warnings appear
- Verify that patron pages behave as before

(Solution as of comment #18)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Verified that warning disappears with this patch.

Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 25 Brendan Gallagher 2016-01-27 02:13:57 UTC
Pushed to Master - Should be in the May 2016 Release. Thanks!
Comment 26 Julian Maurice 2016-01-29 10:50:24 UTC
Patch pushed to 3.22.x, will be in 3.22.3