Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix send_file environment variables #1067

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

odyniec
Copy link
Member

@odyniec odyniec commented Sep 14, 2014

Some request environment variables, such as psgi.*, are missing when a file is served using send_file. I'm not sure how to write a proper test for this, but the problem can be demonstrated with a simple app:

package testenv;
use Dancer ':syntax';

our $VERSION = '0.1';

get '/' => sub {
    template 'index';
};

get '/file' => sub {
    send_file('/etc/hostname', system_path => 1, content_type => 'text/plain');
};

hook 'after_file_render' => sub {
    debug "url_scheme: " . request->env->{'psgi.url_scheme'};
};

true;

A request for, say, /images/perldancer.jpg, produces url_scheme: http, while a request for /file results in url_scheme: (undefined).

This change fixes the problem by passing the SharedData->request environment to the new request object created in _send_file.

@jwittkoski
Copy link
Contributor

In the existing code a minimal replacement request object is created (it's simply a GET $path with nothing else). Unfortunately, this drops certain headers that were already computed in the original request object.

Why doesn't it just take the original request object, and just use method() and path() to override the values instead?

@jwittkoski
Copy link
Contributor

This bug also shows up in the logs if you are using certain logging placeholders (like %h) which are populated from the (non-existent) environment. For example, the client's IP/host will show up as - in the logs for any responses that use send_file.

I didn't realize it at the time, but this bug is at least one reason #1029 was happening in some situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants