Bug 8311 - Mis-scoped function call in C4::Auth
Summary: Mis-scoped function call in C4::Auth
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low normal (vote)
Assignee: Jared Camins-Esakov
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-25 16:06 UTC by Jared Camins-Esakov
Modified: 2013-12-05 19:59 UTC (History)
4 users (show)

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


Attachments
Bug 8311: Fix scoping error in C4::Auth (2.05 KB, patch)
2012-06-25 16:09 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8311: Fix scoping error in C4::Auth (2.10 KB, patch)
2012-06-27 06:30 UTC, Marc Véron
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2012-06-25 16:06:23 UTC
There is a mis-scoped function call in C4::Auth, on line 154, where GetMembers is called without explicit scoping and before 'require C4::Members;'. This does not actually have any functional ramifications as far as I can tell, but it would be a good idea to fix it.
Comment 1 Jared Camins-Esakov 2012-06-25 16:09:34 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2012-06-27 06:30:24 UTC
Created attachment 10532 [details] [review]
[SIGNED-OFF] Bug 8311: Fix scoping error in C4::Auth

There is a mis-scoped function call in C4::Auth, on line 154, where
GetMembers is called without explicit scoping and before
'require C4::Members;'. This does not actually have any functional
ramifications as far as I can tell, but it would be a good idea to fix
it.

This patch also corrects a bit of indenting in that area, because it was
an unnecessary challenge to understand the code with the mis-indenting.

Signed-off-by: Marc Veron <veron@veron.ch>
Comment 3 Jonathan Druart 2012-06-27 10:15:02 UTC
I don't know how we can reproduce the error. But the patch fixes a mis-scoped function.

Marking as Passed QA
Comment 4 Paul Poulain 2012-06-29 09:23:47 UTC
QA comment:

Why did you put the line 
        require C4::Members;
outside from
        if (!defined($borrowernumber) && defined($user)) {
?

The way you made it, C4::Members will be loaded on everypage, that will impact performances !

Written:
    if (!defined($borrowernumber) && defined($user)) {
        require C4::Members;
        my $borrower = C4::Members::GetMember(borrowernumber => $user);
will load the C4::Members only if the getborrowernumber does not return the user (which should never happen)

waiting for your feedback, not pushing not marking failed QA
Comment 5 Jared Camins-Esakov 2012-06-29 10:06:48 UTC
(In reply to comment #4)
> QA comment:
> 
> Why did you put the line 
>         require C4::Members;
> outside from
>         if (!defined($borrowernumber) && defined($user)) {
> ?
> 
> The way you made it, C4::Members will be loaded on everypage, that will
> impact performances !

C4::Members was always loaded whenever $user was set. I just moved the require up so that it was before *both* calls to routines in C4::Members (if you scroll down about a page, you'll see a call to GetMemberDetails.

> Written:
>     if (!defined($borrowernumber) && defined($user)) {
>         require C4::Members;
>         my $borrower = C4::Members::GetMember(borrowernumber => $user);
> will load the C4::Members only if the getborrowernumber does not return the
> user (which should never happen)
> 
> waiting for your feedback, not pushing not marking failed QA
Comment 6 Paul Poulain 2012-06-29 14:17:31 UTC
OK, got it. Patch pushed
Comment 7 Chris Cormack 2012-06-29 21:48:32 UTC
Pushed to 3.8.x, will be in 3.8.3