Bug 7769

Summary: members/pay.pl: $template scoping for plack
Product: Koha Reporter: Dobrica Pavlinusic <dpavlin>
Component: CirculationAssignee: Dobrica Pavlinusic <dpavlin>
Status: CLOSED FIXED QA Contact: Ian Walls <koha.sekjal>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, matthias.meusburger, paul.poulain
Version: 3.8   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7172    
Attachments: proposed patch
Bug 7769 - members/pay.pl: $template scoping for plack
Bug 7769 - members/pay.pl: $template scoping for plack

Description Dobrica Pavlinusic 2012-03-21 10:03:03 UTC Comment hidden (obsolete)
Comment 1 Dobrica Pavlinusic 2012-03-21 14:49:09 UTC Comment hidden (obsolete)
Comment 2 Matthias Meusburger 2012-03-21 15:57:09 UTC
Testing ok, signing off
Comment 3 Matthias Meusburger 2012-03-21 15:57:33 UTC
Created attachment 8445 [details] [review]
Bug 7769 - members/pay.pl: $template scoping for plack

Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com>
Comment 4 Ian Walls 2012-03-22 00:57:30 UTC
Okay, this is a very simple change, 2 char for 3, but I need a little more knowledge before I can QA it.

We're changing 'my' to 'our'.  This increases the scope of the 3 variables, $template, $loggedinuser and $cookie to be global to the module, which makes sense.  I *think* we need to do this because several subroutines within members/pay.pl alter $template, and the way that plack or mod_perl work more explicit scoping is required.  Is this the case?  Are there any side effects of changing this scope for a non-persistent setup?
Comment 5 Dobrica Pavlinusic 2012-03-22 08:13:20 UTC
I don't think there are any side-effect on non-persistent setup, and indeed we are doing this because we reference it inside subs.

It would be enough to scope just $template, and to be pedantic, we are defining three global variables instead of one which does increase memory usage in persistent environment, but alternatives would require larger code changes.
Comment 6 Ian Walls 2012-03-22 12:58:29 UTC

Thanks for the info.  Looks like I'm not as far behind as I feared.  While this does global scope 3 variables instead of just one, it's only two more variables (how many bytes is that?), and the code change is easier to read, understand and QA.

Given this, I'm going to pass this patch through QA.  I'll give some time to test for strange, unexpected changes in behavior, and assuming everything behaves like we want, I'll start passing the rest of the plack variable scoping changes.
Comment 7 Paul Poulain 2012-03-22 15:27:40 UTC
Ian, if you want/need more info:
and more specifically:
Second day of plack testing showed that we are somewhat away from goal of running intranet under plack. Currently biggest problems are variabled declared with my which are inside sub (since plack wraps our cgi scripts within sub). This problem is similar to mod_perl problem described at http://modperlbook.org/html/6-2-Exposing-Apache-Registry-Secrets.html (just ignore mod_perl references :-) 

I find this solution a little bit dirty, but, oth, the speed under Plack is so... astonishing, that my feeling is nothing ;-) (gain = speed x5, outside from zebra speed/queries)

Note : most of the patch dobrika made yesterday and today fix the problem that way.
If you havent QAed them, i'll QA now that you agree

Just in case = this patch should not be applied on 3.6.
Comment 8 Jared Camins-Esakov 2012-12-31 00:45:47 UTC
There have been no further reports of problems so I am marking this bug resolved.