Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix env #874

Merged
merged 2 commits into from

3 participants

@kentfredric

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.

kentfredric added some commits
@kentfredric kentfredric Change regexps using { to have { escaped for compatibily with perl >=…
…5.17

Solves gh #872
ab8de23
@kentfredric kentfredric 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`.
4f3d33b
@bigpresh
Owner

Ahh, good work, thanks! I'd been looking at that same failure with 5.17.6, but couldn't figure out what was going on. Thanks for getting to the bottom of it!

@bigpresh
Owner

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?

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.

Owner

Sane and detailed explanation, thanks!

@bigpresh bigpresh merged commit 6bac57e into PerlDancer:devel
@yanick yanick referenced this pull request
Closed

Test failure on 5.17.6 #870

@mokko

I checked first two patches and this PR appears to be merged already; can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 15, 2012
  1. @kentfredric
  2. @kentfredric

    Reduce dependence on %ENV for internal code.

    kentfredric authored
    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`.
This page is out of date. Refresh to see the latest.
View
4 lib/Dancer/Logger/Abstract.pm
@@ -124,12 +124,12 @@ sub format_message {
my $fmt = $self->_log_format();
- $fmt =~ s{
+ $fmt =~ s^
(?:
\%\{(.+?)\}([a-z])|
\%([a-zA-Z])
)
- }{ $1 ? $block_handler->($1, $2) : $char_mapping->($3) }egx;
+ ^ $1 ? $block_handler->($1, $2) : $char_mapping->($3) ^egx;
return $fmt."\n";
}
View
17 lib/Dancer/Request.pm
@@ -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
11 lib/Dancer/Test.pm
@@ -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
View
4 t/00_base/14_changelog.t
@@ -102,8 +102,8 @@ if ( (my ($pre, $version, $post)) = ($line =~ /^(\s*)(\S.*\S)(\s*)$/)) {
my $lpost = length $post;
$lpre and _fail("line starts with $lpre blank caracters, but it should not");
$lpost and _fail("line ends with $lpre blank caracters, but it should not");
- like($version, qr/^{{\$NEXT}}$|^\d\.\d{4}(_\d{2} | )\d{2}.\d{2}.\d{4}$/, "changelog line $line_nb: check version failed");
- $version =~ qr/^({{\$NEXT}})$|^\d\.\d{4}(_\d{2} | )\d{2}.\d{2}.\d{4}$/;
+ like($version, qr/^\{\{\$NEXT\}\}$|^\d\.\d{4}(_\d{2} | )\d{2}.\d{2}.\d{4}$/, "changelog line $line_nb: check version failed");
+ $version =~ qr/^(\{\{\$NEXT\}\})$|^\d\.\d{4}(_\d{2} | )\d{2}.\d{2}.\d{4}$/;
# print STDERR " -------> [$1] [$2]\n";
$current_version_is_dev = defined $1 || $2 =~ /^_\d{2}/;
View
4 t/01_config/06_stack_trace.t
@@ -25,7 +25,7 @@ use Dancer::Template::TemplateToolkit;
is(scalar(@error_lines), 3, "test verbose croak");
like($error_lines[0], qr!^core - template - '/not/a/valid/file' doesn\'t exist or not a regular file at!, "test verbose croak");
like($error_lines[1], qr!^\s*Dancer::Template::TemplateToolkit::render\('Dancer::Template::TemplateToolkit', '/not/a/valid/file'\) called at!, "test verbose croak stack trace");
- like($error_lines[2], qr!^\s*eval {...} called at (?:[.]/)?t/01_config/06_stack_trace.t!, "test verbose croak stack trace");
+ like($error_lines[2], qr!^\s*eval \{...\} called at (?:[.]/)?t/01_config/06_stack_trace.t!, "test verbose croak stack trace");
}
{
@@ -49,5 +49,5 @@ use Dancer::Template::TemplateToolkit;
is(scalar(@error_lines), 3, "test verbose croak");
like($error_lines[0], qr!^core - template - '/not/a/valid/file' doesn\'t exist or not a regular file at!, "test verbose croak");
like($error_lines[1], qr!^\s*Dancer::Template::TemplateToolkit::render\('Dancer::Template::TemplateToolkit', '/not/a/valid/file'\) called at!, "test verbose croak stack trace");
- like($error_lines[2], qr!^\s*eval {...} called at (?:[.]/)?t/01_config/06_stack_trace.t!, "test verbose croak stack trace");
+ like($error_lines[2], qr!^\s*eval \{...\} called at (?:[.]/)?t/01_config/06_stack_trace.t!, "test verbose croak stack trace");
}
View
4 t/11_logger/08_serialize.t
@@ -20,7 +20,7 @@ Test::Output::stderr_like(
Test::Output::stderr_like(
sub { Dancer::Logger::warning( { this => 'that' } ) },
- qr/\[\d+\] warn @.+> {'this' => 'that'} in/,
+ qr/\[\d+\] warn @.+> \{'this' => 'that'\} in/,
'Hashref correctly serialized',
);
@@ -32,6 +32,6 @@ Test::Output::stderr_like(
Test::Output::stderr_like(
sub { Dancer::Logger::warning( { b => 1, a => 2, e => 3, d => 4, c => 5}) },
- qr/\[\d+\] warn @.+> {'a' => 2,'b' => 1,'c' => 5,'d' => 4,'e' => 3}/,
+ qr/\[\d+\] warn @.+> \{'a' => 2,'b' => 1,'c' => 5,'d' => 4,'e' => 3\}/,
'Hash keys are sorted okay',
);
View
2  t/14_serializer/01_helpers.t
@@ -68,7 +68,7 @@ SKIP: {
# with options
$data = { foo => { bar => { baz => [qw/23 42/] } } };
$json = to_json( $data, { pretty => 1 } );
- like $json, qr/"foo" : {/, "data is pretty!";
+ like $json, qr/"foo" : \{/, "data is pretty!";
my $data2 = from_json($json);
is_deeply( $data2, $data, "data is correctly deserialized" );
Something went wrong with that request. Please try again.