Bug 7769 - members/pay.pl: $template scoping for plack
Summary: members/pay.pl: $template scoping for plack
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Dobrica Pavlinusic
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks: 7172
  Show dependency treegraph
 
Reported: 2012-03-21 10:03 UTC by Dobrica Pavlinusic
Modified: 2013-12-05 20:05 UTC (History)
3 users (show)

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


Attachments
proposed patch (660 bytes, patch)
2012-03-21 10:03 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 7769 - members/pay.pl: $template scoping for plack (795 bytes, patch)
2012-03-21 14:49 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 7769 - members/pay.pl: $template scoping for plack (865 bytes, patch)
2012-03-21 15:57 UTC, Matthias Meusburger
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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
Dobrica,

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:
http://wiki.koha-community.org/wiki/Plack#Running_Intranet
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.