Bug 10126 - C4::Reports::Guided is not Plack safe
Summary: C4::Reports::Guided is not Plack safe
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Galen Charlton
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 7172
  Show dependency treegraph
 
Reported: 2013-04-26 12:06 UTC by Jared Camins-Esakov
Modified: 2016-08-15 14:45 UTC (History)
3 users (show)

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


Attachments
Bug 10126: Remove my variables at module level (8.22 KB, patch)
2014-10-09 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10126: Remove my variables at module level (8.35 KB, patch)
2014-10-09 15:52 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10126: Remove my variables at module level (8.41 KB, patch)
2014-10-10 19:44 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 10126: (qa followup) fix tests (1005 bytes, patch)
2014-10-16 13:26 UTC, Tomás Cohen Arazi
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 2013-04-26 12:06:24 UTC
Notice then numerous my variables at module level. There is also a call to C4::Context->preference in a BEGIN block, which is bad.
Comment 1 Jonathan Druart 2014-10-09 15:20:28 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-10-09 15:20:59 UTC
This patch is quite ugly, feel free to provide your own version ;)
Comment 3 Chris Cormack 2014-10-09 15:52:22 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2014-10-10 19:44:16 UTC
QA comments :
 * passes koha-qa.pl
 * guided reports also still work in CGI mode
 * no other comments, looks good

passed QA
Comment 5 Paul Poulain 2014-10-10 19:44:51 UTC
Created attachment 32199 [details] [review]
Bug 10126: Remove my variables at module level

In C4::Reports::Guided, a lot of variables was defined at module level
and reused in subroutine.

I didn't find any problem with Plack, so I'm not sure this patch is
useful.

Test plan:
Verify there is no more ^my in the module and you don't find any
regression with the guided reports tools (with and without Plack)

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

I'm unsure if this is needed, but I have verified it causes no
regressions

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 6 Tomás Cohen Arazi 2014-10-15 21:27:54 UTC
Patch pushed to master.

Thanks Jonathan.
Comment 7 Tomás Cohen Arazi 2014-10-16 13:26:28 UTC
Created attachment 32409 [details] [review]
Bug 10126: (qa followup) fix tests

It seems the removal of global variables changes the behaviour
of Test::MockModule, and it now expects the namespace in front
of the statically called method.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>