Bug 10900

Summary: Incorrect calling conventions accessing C4::Context
Product: Koha Reporter: Mark Tompsett <mtompset>
Component: Architecture, internals, and plumbingAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: gitbot, gmcharlt, jonathan.druart, kyle, martin.renvoize, srdjan, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14325
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 10589, 10895    
Attachments: Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Incorrect calling conventions accessing C4::Context
Correct calling convention for set_userenv
Fix t/Circulation_barcodedecode.t set_userenv calls
Fix other set_userenv calls with bad parameters
Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Follow up, since more has been added to master
Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Follow up, since more has been added to master
Bug 10900 - Follow up, since more has been added to master
Bug 10900: 2 occurrences more
Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Follow up, since more has been added to master
Bug 10900: 2 occurrences more
Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Follow up, since more has been added to master
Bug 10900: 2 occurrences more
Bug 10900 - Incorrect calling conventions accessing C4::Context
Bug 10900 - Follow up, since more has been added to master
Bug 10900: 2 occurrences more

Description Mark Tompsett 2013-09-17 14:08:41 UTC
While trying to write tests (bug 10895), I was frustrated for an excessively long time while trying to copy t/db_dependent/Members.t as a template. The problem became apparent when I actually stop to note the difference between a :: over a -> in the calling convention.

C4::Context->method passes the object as the first parameter. C4::Context::function calls the routine in a purely functional way.

C4::Context methods should be called in an object oriented manner, particularly things like C4::Context::set_userenv. Set/Get are very much OO things!

As pieces get fixed, the patches will drop here.
Comment 1 Mark Tompsett 2013-09-18 04:05:00 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2013-09-18 04:15:06 UTC
The validity of the change was checked with:
    $ prove -v t/db_dependent/Circulation.t

If it works for that case, it should work for all cases.

I was able to log in, and seeing as that set_userenv is in Auth, I figured it got triggered there too.

I had a DB that was two versions behind, and I triggered an upgrade on the staff client successfully too.
Comment 3 Mark Tompsett 2013-09-18 05:09:47 UTC
How to trigger all the set_userenv's in C4/Auth.pm:
log in using userid (triggers 1, 2, 6)
log in using cardnumber (triggers 7 and others)
library-intra.mydnsname.org/cgi-bin/koha/svc/bib/{bibnumber}?userid={userid}&password={password} (first time triggers 4)
library-intra.mydnsname.org/cgi-bin/koha/svc/bib/{bibnumber} (once logged in triggering 4, this triggers 3)
tools -> tags -> type something in the "Check lists" text box and click 'test'. (triggers 5)

To trigger the set_userenv's in installer/InstallAuth.pm, just run an upgrade of a database from a previous version to the current master, and redirect to the log in screen.

And having figured all this out... It worked.
Comment 4 Mark Tompsett 2013-09-20 03:33:42 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2013-09-20 19:39:04 UTC
A little more to come... Another set of side related problems. *sigh*
Comment 6 Mark Tompsett 2013-09-21 04:31:36 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2013-09-21 04:32:47 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2013-09-21 04:33:31 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2013-09-21 04:36:17 UTC
How to trigger all the set_userenv's in C4/Auth.pm:
log in using userid (triggers 1, 2, 6)
log in using cardnumber (triggers 7 and others)
library-intra.mydnsname.org/cgi-bin/koha/svc/bib/{bibnumber}?userid={userid}&password={password} (first time triggers 4)
library-intra.mydnsname.org/cgi-bin/koha/svc/bib/{bibnumber} (once logged in triggering 4, this triggers 3)
tools -> tags -> type something in the "Check lists" text box and click 'test'. (triggers 5)

To trigger the set_userenv's in installer/InstallAuth.pm, just run an upgrade of a database from a previous version to the current master, and redirect to the log in screen.

prove -v t/Circulation_barcodedecode.t

I have yet to determine how to trigger the patched areas in the third patch.
Comment 10 Mark Tompsett 2013-09-22 03:22:03 UTC
Okay, I believe there are 19 change points in these three patches. To trigger these change points (ie. Here is a test plan):
koha staff client - login  userid      - 2,6,7,8,14
koha staff client - logout             - 1
koha staff client - login cardnumber   - 9,10,11
prove -v t/Circulation_barcodedecode.t - 18
prove -v t/db_dependent/Circulation.t  - 19

http://{staff client url}/cgi-bin/koha/svc/bib/{biblionumber}?userid={userid}&password={password}                    - 4

koha staff client - login  userid  AND then
http://{staff client url}/cgi-bin/koha/svc/bib/{biblionumber}
                                       - 3
tools -> tags -> type something in the "Check lists" text box and click 'test'.                                - 5

koha staff client - login  system user - 12

perldoc C4::Context
/set_userenv                           - 13

koha staff client - upgrade database   - 16,17,15
Comment 11 I'm just a bot 2013-09-24 05:36:47 UTC
Patch applied cleanly, go forth and signoff
Comment 12 Srdjan Jankovic 2013-10-03 00:41:28 UTC
I'm not sure this is correct. C4::Context is not an object, hence has no methods. I believe :: is more appropriate under the circumstances. Although we may agree to call C4::Context subs using ->, but that would be just a waste of the first argument.

Context *should* be an object, I agree, but unfortunately it is not. Is this being addressed in Koha:: namespace?
Comment 13 Mark Tompsett 2013-10-03 01:09:33 UTC
I don't know if it is being addressed in the Koha:: namespace. However, if someone is working on splitting C4::Context, this means that one piece of the split up is closer to being the object it should be. See bug 7176.
Comment 14 Srdjan Jankovic 2013-10-03 01:16:42 UTC
Ok, then I'll rephrase:
If Context is moving to Koha, then this is not necessary. If it is not, well then I can see the point.
Comment 15 Mark Tompsett 2013-10-17 19:54:23 UTC Comment hidden (obsolete)
Comment 16 Mark Tompsett 2013-10-17 19:59:42 UTC
Had to rebase due to the changed hashing algorithm of the passwords patch which has made it into master.
Comment 17 Mark Tompsett 2013-10-17 20:05:50 UTC Comment hidden (obsolete)
Comment 18 Mark Tompsett 2013-10-25 18:51:47 UTC
The test plan is in comment #10.
Comment 19 Kyle M Hall 2013-10-25 19:05:45 UTC
Looks like this patch is causing Circulation_barcodedecode.t to fails some tests. All the unit tests pass on master.

perl t/Circulation_barcodedecode.t
1..26
ok 1 - use C4::Circulation;
ok 2 -        EAN13:       '892685001928' => '0892685001928'
ok 3 -        EAN13:             '695152' => '0000000695152'
ok 4 -     T-prefix:           'T0031472' =>      'T0031472'
ok 5 -     T-prefix:                'T32' =>      'T0000002'
ok 6 -       cuecat:           '26002315' =>      '26002315'
ok 7 -       cuecat: '.C3nZC3nZC3nYD3b6ENnZCNnY.fHmc.C3D1Dxr2C3nZE3n7.' =>  '046675000808'
ok 8 -       cuecat: '.C3nZC3nZC3nYD3b6ENnZCNnY.fHmc.C3D1Dxr2C3nZE3n7.
# ' =>  '046675000808'
ok 9 -       cuecat: 'q.C3nZC3nZC3nWDNzYDxf2CNnY.fHmc.C3DWC3nZCNjXD3nW.' =>  '043000112403'
ok 10 -       cuecat: '.C3nZC3nZC3nWCxjWE3D1C3nX.cGf2.ENr7C3v7D3T3ENj3C3zYDNnZ.' => '978068484914051500'
not ok 11 -    libsuite8:            'b000126' =>     'IMS-b-126'
#   Failed test '   libsuite8:            'b000126' =>     'IMS-b-126''
#   at t/Circulation_barcodedecode.t line 38.
# Bad output: 'kale-b-126'
not ok 12 -    libsuite8:                'b12' =>      'IMS-b-12'
#   Failed test '   libsuite8:                'b12' =>      'IMS-b-12''
#   at t/Circulation_barcodedecode.t line 38.
# Bad output: 'kale-b-12'
not ok 13 -    libsuite8:              'B0126' =>     'IMS-B-126'
#   Failed test '   libsuite8:              'B0126' =>     'IMS-B-126''
#   at t/Circulation_barcodedecode.t line 38.
# Bad output: 'kale-B-126'
not ok 14 -    libsuite8:          'IMS-B-126' =>     'IMS-B-126'
#   Failed test '   libsuite8:          'IMS-B-126' =>     'IMS-B-126''
#   at t/Circulation_barcodedecode.t line 38.
# Bad output: 'kale-IMS-B--126'
not ok 15 -    libsuite8:          'ims-b-126' =>     'ims-b-126'
#   Failed test '   libsuite8:          'ims-b-126' =>     'ims-b-126''
#   at t/Circulation_barcodedecode.t line 38.
# Bad output: 'kale-ims-b--126'
not ok 16 -    libsuite8:          'CD0000024' =>     'IMS-CD-24'
#   Failed test '   libsuite8:          'CD0000024' =>     'IMS-CD-24''
#   at t/Circulation_barcodedecode.t line 38.
# Bad output: 'kale-CD-24'
not ok 17 -    libsuite8:              '00123' =>     'IMS-b-123'
#   Failed test '   libsuite8:              '00123' =>     'IMS-b-123''
#   at t/Circulation_barcodedecode.t line 38.
# Bad output: 'kale-b-123'
not ok 18 -    libsuite8:              '11998' =>   'IMS-b-11998'
#   Failed test '   libsuite8:              '11998' =>   'IMS-b-11998''
#   at t/Circulation_barcodedecode.t line 38.
# Bad output: 'kale-b-11998'
ok 19 -        other:           '26002315' =>      '26002315'
ok 20 -        other:           'T0031472' =>      'T0031472'
ok 21 -        other:                'T32' =>           'T32'
ok 22 -        other:        'Alphanum123' =>   'Alphanum123'
ok 23 -        other:      'Alpha Num 345' => 'Alpha Num 345'
ok 24 -   whitespace:          ' 26002315' =>      '26002315'
ok 25 -   whitespace:          '26002315 ' =>      '26002315'
ok 26 -   whitespace:        '
#       26002315
# ' =>      '26002315'
# Looks like you failed 8 tests of 26.
Comment 20 Mark Tompsett 2013-10-26 02:02:30 UTC
After reading your post, Kyle, I did a fresh git install on a fresh OS clone. I could not replicate your failure.

mtompset@ubuntu:~/kohaclone$ git checkout -b bug_10900 origin/master
Branch bug_10900 set up to track remote branch master from origin.
Switched to a new branch 'bug_10900'
mtompset@ubuntu:~/kohaclone$ git diff origin/master
mtompset@ubuntu:~/kohaclone$ git bz apply 10900
Bug 10900 - Incorrect calling conventions accessing C4::Context

Bug 10900 - Incorrect calling conventions accessing C4::Context
Apply? [yn] y

Applying: Bug 10900 - Incorrect calling conventions accessing C4::Context
mtompset@ubuntu:~/kohaclone$ prove -v t/Circulation_barcodedecode.t
t/Circulation_barcodedecode.t ..
1..26
ok 1 - use C4::Circulation;
ok 2 -        EAN13:       '892685001928' => '0892685001928'
ok 3 -        EAN13:             '695152' => '0000000695152'
ok 4 -     T-prefix:           'T0031472' =>      'T0031472'
ok 5 -     T-prefix:                'T32' =>      'T0000002'
ok 6 -       cuecat:           '26002315' =>      '26002315'
ok 7 -       cuecat: '.C3nZC3nZC3nYD3b6ENnZCNnY.fHmc.C3D1Dxr2C3nZE3n7.' =>  '046675000808'
ok 8 -       cuecat: '.C3nZC3nZC3nYD3b6ENnZCNnY.fHmc.C3D1Dxr2C3nZE3n7.
# ' =>  '046675000808'
ok 9 -       cuecat: 'q.C3nZC3nZC3nWDNzYDxf2CNnY.fHmc.C3DWC3nZCNjXD3nW.' =>  '043000112403'
ok 10 -       cuecat: '.C3nZC3nZC3nWCxjWE3D1C3nX.cGf2.ENr7C3v7D3T3ENj3C3zYDNnZ.' => '978068484914051500'
ok 11 -    libsuite8:            'b000126' =>     'IMS-b-126'
ok 12 -    libsuite8:                'b12' =>      'IMS-b-12'
ok 13 -    libsuite8:              'B0126' =>     'IMS-B-126'
ok 14 -    libsuite8:          'IMS-B-126' =>     'IMS-B-126'
ok 15 -    libsuite8:          'ims-b-126' =>     'ims-b-126'
ok 16 -    libsuite8:          'CD0000024' =>     'IMS-CD-24'
ok 17 -    libsuite8:              '00123' =>     'IMS-b-123'
ok 18 -    libsuite8:              '11998' =>   'IMS-b-11998'
ok 19 -        other:           '26002315' =>      '26002315'
ok 20 -        other:           'T0031472' =>      'T0031472'
ok 21 -        other:                'T32' =>           'T32'
ok 22 -        other:        'Alphanum123' =>   'Alphanum123'
ok 23 -        other:      'Alpha Num 345' => 'Alpha Num 345'
ok 24 -   whitespace:          ' 26002315' =>      '26002315'
ok 25 -   whitespace:          '26002315 ' =>      '26002315'
ok 26 -   whitespace:        '
#       26002315
# ' =>      '26002315'
ok
All tests successful.
Files=1, Tests=26,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.30 cusr  0.02 csys =  0.34 CPU)
Result: PASS

So, I looked at your bad output line:
# Bad output: 'kale-b-126'

'kale' is the last name in the set_userenv call.
The line that changed in the test file is:
C4::Context->set_userenv(1,'kmkale' , 1, 'km', 'kale' , 'IMS', 'IMS Branch DEscription', 0, 'kmkale@anantcorp.com');

This would be affected if the set_userenv function in C4::Context is missing shift @_; or the call is :: in the test file.

From C4::Context:
    shift @_;
    my ($usernum, $userid, $usercnum, $userfirstname, $usersurname, $userbranch, $branchname, $userflags, $emailaddress, $branchprinter, $persona)= @_;

The -> passes an object, which is shifted off, and $usernum=1,$userid='kmkale',$usercnum=1,$userfirstname='km',$usersurname='kale',$userbranch='IMS'

The barcodedecode function uses the userbranch. So, if there is a missing shift @_ and the Circulationbarcode.t file has ->set_userenv(...) as above, then that $userbranch would be 'kale', which matches your failed output. The 'kale-' portion is based on the branchcode.

From C4/Circulation.pm
sub barcodedecode {
    my ($barcode, $filter) = @_;
    my $branch = C4::Branch::mybranch();
...
        } elsif ($filter eq 'libsuite8') {
                unless($barcode =~ m/^($branch)-/i){    #if barcode starts with branch code its in Koha style. Skip it.
                        if($barcode =~ m/^(\d)/i){      #Some barcodes even start with 0's & numbers and are assumed to have b as the item type in the libsuite8 software
                                $barcode =~ s/^[0]*(\d+)$/$branch-b-$1/i;
                        }else{
                                $barcode =~ s/^(\D+)[0]*(\d+)$/$branch-$1-$2/i;
                        }
                }

From C4/Branch.pm
# always returns a string for OK comparison via "eq" or "ne"
sub mybranch {
    C4::Context->userenv           or return '';
    return C4::Context->userenv->{branch} || '';
}
Comment 21 Kyle M Hall 2013-10-30 12:35:04 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2013-11-14 16:01:07 UTC
QA comment:

All looks good, no regression found (I did not launch all UT).
The persona login works.
As Srdjan already said, I am not sure the set_userenv routine should be used as a method. But no importance when looking at the C4::Context module...

I would have been nice to make the most of the opportunity to replace the parameter list with a hashref.

Marked as Passed QA.
Comment 23 Jonathan Druart 2013-11-14 16:01:23 UTC Comment hidden (obsolete)
Comment 24 Mark Tompsett 2013-11-14 16:05:35 UTC
(In reply to Jonathan Druart from comment #22)
> I would have been nice to make the most of the opportunity to replace the
> parameter list with a hashref.

I think the fixing the parameters to hashrefs would be better done in bug 7176.
Comment 25 Mark Tompsett 2014-04-28 16:57:22 UTC Comment hidden (obsolete)
Comment 26 Galen Charlton 2014-05-05 23:09:15 UTC
Punting to next release as it's tied to bug 10589.
Comment 27 Mark Tompsett 2014-05-12 21:49:29 UTC
Actually, it is tied to bug 10589 only in that it is required for it to work. This actually changes no functionality whatsoever.

It tidies calls to functions in C4::Context, making them called the same way. That is, use -> instead of :: in C4::Context function calls, particularly this piece which corrects set_userenv. It also corrects a few bugs which are working by accident, because the parameter list is wrong.
Comment 28 Mark Tompsett 2014-05-12 22:03:25 UTC Comment hidden (obsolete)
Comment 29 Mark Tompsett 2014-07-09 20:25:20 UTC
Even if 10589 and 10895 don't make it into Koha, I think this refactor/clean up is still useful to do. There are some "works by mistake" changes in here.
Comment 30 Mark Tompsett 2014-07-29 03:10:22 UTC Comment hidden (obsolete)
Comment 31 Mark Tompsett 2014-07-29 03:10:37 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2014-08-29 15:29:25 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2014-08-29 15:29:32 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2014-08-29 15:31:06 UTC
Sorry, I forgot this one.

Need QA!
Comment 35 Mark Tompsett 2014-09-11 16:52:13 UTC
See http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10589#c34 for how this all relates piecewise to the other bugs.
Comment 36 Jonathan Druart 2014-09-16 13:52:35 UTC
On QAing the 3 bug reports:

The Circulation UT file is broken:
 prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. 2/51 Undefined subroutine &C4::Circulation::GetItem called at C4/Circulation.pm line 1204.
Comment 37 Mark Tompsett 2014-09-16 15:08:31 UTC
(In reply to Jonathan Druart from comment #36)
> On QAing the 3 bug reports:
> 
> The Circulation UT file is broken:
>  prove t/db_dependent/Circulation.t
> t/db_dependent/Circulation.t .. 2/51 Undefined subroutine
> &C4::Circulation::GetItem called at C4/Circulation.pm line 1204.

Hmmm... this is not because of this bug report, which can be QA'd by itself.
comment #10
comment #32
prove -v t/db_dependent/Members.t

I will try to figure out the mess on the other two bugs. Setting this to Signed Off, since this bug can be QA'd seperately.
Comment 38 Jonathan Druart 2014-09-18 15:30:11 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2014-09-18 15:30:18 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2014-09-18 15:30:26 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2014-09-18 15:31:18 UTC
I will try to add a rule in the qa tools when pushed.
Comment 42 Mark Tompsett 2014-12-12 15:06:43 UTC Comment hidden (obsolete)
Comment 43 Mark Tompsett 2014-12-12 15:06:50 UTC Comment hidden (obsolete)
Comment 44 Mark Tompsett 2014-12-12 15:06:57 UTC Comment hidden (obsolete)
Comment 45 Mark Tompsett 2014-12-12 15:09:23 UTC
Because of the new shibboleth parameter to set_userenv in C4::Context, a rebase was required. There should be no other visible changes between the last two versions of " Bug 10900 - Incorrect calling conventions accessing C4::Context". Leaving as Passed QA.
Comment 46 Martin Renvoize 2015-01-26 17:03:06 UTC
Created attachment 35542 [details] [review]
Bug 10900 - Incorrect calling conventions accessing C4::Context

There were multiple calling conventions for C4::Context's
set_userenv routine. So the following commands were used to
find discrepancies:
    grep "::set_userenv" `find .`
    grep "\->set_userenv" `find .`

The first grep demonstrated that the smaller change is from
:: to -> as only C4/Auth.pm, installer/InstallAuth.pm, and
t/db_dependent/Circulation.t would need to be modified. This
patch corrects C4::Context's set_userenv routine to be object
call based (use ->) by using a shift to ignore the first
parameter, and modify the three files found with :: calls.

As the result of trying to roll a distribution,
t/Circulation_barcodedecode.t was discovered to be faulty. The
cause being incorrect parameters! This was hidden when there
was no shift in the set_userenv routine. However, with its
correction, the test broke.

This led me to read the POD documentation for the function
set_userenv in C4::Context and realize it was outdated as
well. It has been revised to match the current version of
the function.

Then intentionally bad parameters passed to the set_userenv
routine in C4::Context were hunted down. The biggest problems
were missing surnames or branch names.

Rebase required because of shibboleth change in C4/Context.pm

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2015-01-26 17:03:11 UTC
Created attachment 35543 [details] [review]
Bug 10900 - Follow up, since more has been added to master

Booksellers.t and Koha_template_plugin_Branches.t both had
function calls to ::set_userenv added to them. This patch handles
those additions.

TEST PLAN
---------
1) Branch a new git branch
2) prove -v t/db_dependent/Bookseller.t
   -- It should work.
3) prove -v t/db_dependent/Koha_template_plugin_Branches.t
   -- It should work.
4) Apply only the first patch.
5) Repeat steps 2 and 3.
   -- They should both FAIL!
6) Apply the second patch as well.
7) Repeat steps 2 and 3.
   -- The should both work.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2015-01-26 17:03:17 UTC
Created attachment 35544 [details] [review]
Bug 10900: 2 occurrences more

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2015-01-26 17:04:18 UTC
Rebased after auth.pm tidy in bug 13499
Comment 50 Martin Renvoize 2015-01-26 17:06:05 UTC
Pushed to Module Maintainers Branch - https://github.com/mrenvoize/Koha/tree/authentication
Comment 51 Tomás Cohen Arazi 2015-02-09 20:20:00 UTC
Patches pushed to master.

Thanks Mark!