Permalink
Browse files

Reduce dependence on %ENV for internal code.

Perl 5.17.3+ changes behaviour of `%ENV` so values are also stringified.

This change aims to solve that ( gh #870 ) by replacing %ENV based code with
a passed around local hashref, that is passed as the last parameter to `new_for_request()`
instead of relying on the data being passed around via `%ENV`.
  • Loading branch information...
1 parent ab8de23 commit 4f3d33b47625f2f0d5f427ec1d80ecac9d46cfee @kentfredric committed Dec 15, 2012
Showing with 17 additions and 11 deletions.
  1. +11 −6 lib/Dancer/Request.pm
  2. +6 −5 lib/Dancer/Test.pm
View
@@ -138,16 +138,21 @@ sub to_string {
# helper for building a request object by hand
# with the given method, path, params, body and headers.
sub new_for_request {
- my ($class, $method, $uri, $params, $body, $headers) = @_;
- $params ||= {};
+ my ($class, $method, $uri, $params, $body, $headers, $extra_env) = @_;
+ $params ||= {};
+ $extra_env ||= {};
$method = uc($method);
my ( $path, $query_string ) = ( $uri =~ /([^?]*)(?:\?(.*))?/s ); #from HTTP::Server::Simple
- my $req = $class->new(env => { %ENV,
- PATH_INFO => $path,
- QUERY_STRING => $query_string || $ENV{QUERY_STRING} || '',
- REQUEST_METHOD => $method});
+ my $env = {
+ %ENV,
+ %{$extra_env},
+ PATH_INFO => $path,
+ QUERY_STRING => $query_string || $ENV{QUERY_STRING} || '',
+ REQUEST_METHOD => $method
+ };
+ my $req = $class->new(env => $env);
$req->{params} = {%{$req->{params}}, %{$params}};
$req->_build_params();
$req->{_query_params} = $req->{params};
View
@@ -299,6 +299,7 @@ sub _check_header {
sub dancer_response {
my ($method, $path, $args) = @_;
$args ||= {};
+ my $extra_env = {};
if ($method =~ /^(?:PUT|POST)$/) {
@@ -346,9 +347,9 @@ Content-Type: text/plain
my $l = 0;
$l = length $content if defined $content;
open my $in, '<', \$content;
- $ENV{'CONTENT_LENGTH'} = $l;
- $ENV{'CONTENT_TYPE'} = $content_type || "";
- $ENV{'psgi.input'} = $in;
+ $extra_env->{'CONTENT_LENGTH'} = $l;
+ $extra_env->{'CONTENT_TYPE'} = $content_type || "";
+ $extra_env->{'psgi.input'} = $in;
}
my ($params, $body, $headers) = @$args{qw(params body headers)};
@@ -357,12 +358,12 @@ Content-Type: text/plain
unless _isa($headers, "HTTP::Headers");
if ($headers->header('Content-Type')) {
- $ENV{'CONTENT_TYPE'} = $headers->header('Content-Type');
+ $extra_env->{'CONTENT_TYPE'} = $headers->header('Content-Type');
}
my $request = Dancer::Request->new_for_request(
$method => $path,
- $params, $body, $headers
+ $params, $body, $headers, $extra_env
);
# first, reset the current state

3 comments on commit 4f3d33b

Looking at this, it'll make the tests pass, as the psgi.input is being set within Dancer::Test to the just-opened filehandle safely - but will that also work when running under Plack itself?

Owner

kentfredric replied Dec 15, 2012

It should, I mean, in the cases where Plack intends to be run as a slave process, the master can't share a handle via %ENV unless its the same perl process, and the same applies for where Plack is the master sending a psgi.input to a Slave.

( Because ENV is a OS level communication layer, which arbitrary datastructures and references to data structures can't be transported over )

And looking through the Plack code, I can't see anything that would take input from %ENV directly, even Dancer doesn't really use %ENV internally, its just a convenience interface for the new_for_request method as far as I can make out.

And that method literally then simply makes a hash-ref copy of %ENV and then passes-by-reference from then on out ( ie: passes between code contexts via @_ , not via a global variable ) ....

And that Method is not called outside Dancer::Test , and never appears in Plack.

In essence, it appears the total surface area of this specific bug is limited to some bad design choices in testing and a few instrumentation methods designed to make testing easier.

Sane and detailed explanation, thanks!

Please sign in to comment.