Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

File handler ignores new information #550

Closed
xsawyerx opened this Issue · 22 comments

5 participants

@xsawyerx
Owner

While converting from Dancer2::Test to Plack::Test (see #544, #545), @racke and I discovered that Dancer2::Handler::File does not update a few headers (Content-Type, Content-Length, and Last-Modified).

It seems like an optimization, but really, it is a bug.

We already have the fix. I'll put it in a branch and push. The only reason I'm opening the ticket is to document that it's best to avoid Dancer2::Test and write using Plack::Test. A proper documentation will be written, fear not.

@xsawyerx xsawyerx added the Bug label
@xsawyerx
Owner

Fixed by fa27fec.

@xsawyerx xsawyerx closed this
@veryrusty
Owner

@xsawyerx - reopening as this will break current implentation in Core::App->send_file

@veryrusty
Owner

And actually reopen..

@veryrusty veryrusty reopened this
@xsawyerx
Owner

Is there a test showing this?

@veryrusty veryrusty referenced this issue from a commit
@veryrusty veryrusty Remove default_content_type attribute to the Response object.
Moves the logic of what content type to apply by 'default' into the
Response object, resulting in much simplier logic within _dispatch_route.

Big win is that the default content type is not applied until it is actually
required. Ref #550 and #551.
08de179
@veryrusty
Owner

Tests - No.
Documentation - Yes; though it looks like that documentation was copied over from D1 - I'm assuming we want to keep the same send_file interface between the two..

I've put another possible approach in this branch: https://github.com/PerlDancer/Dancer2/tree/feature/refactor-default-content-type . This fails the existing redirect test as Dancer2::Test doesn't call Response->to_psgi that includes some defaults; I haven't verified, but it should pass if called via Plack::Test. (no PR as it fails that test..)

@xsawyerx
Owner

Thanks for the research and work on the branch. :)

Just a naive question here: when do we have a situation in which you want to change the file content-type in a before hook from what we discover it to be?

@xsawyerx xsawyerx added Core-Dev Pending Review and removed Bug labels
@veryrusty
Owner

That's not a naive question! :) Given that Handler::File is always dealing with a file "on disk" and D2 doesn't support send_file(\$data), I can't think of a situation where you need to change the content-type before file render. The after file render hook that can change the content-type if needed too.

I went through the Dancer1 history; @ambs added support for overriding the content-type in send_file 3 years ago.. How goods your memory? ;)

@xsawyerx
Owner

Mine is terrible.

@ambs: do you know why you added this feature?

@ambs
Owner

Yeah. I remember... not :-)
But I am mostly sure it was relevant :angel:
Let me grep my projects for send_file, and see if it rings a bell...

@ambs
Owner

Rereading @veryrusty I recall I was sending some images rendered on the fly with send_file. I think. But really can't recall that...

@racke
Owner
@xsawyerx
Owner

Agreed.

@veryrusty
Owner

I also agree we should support send_file(\$data).

Trying to move this along; how does this sound:

  • Finish the conversion to from Dancer2::Test to Plack::Test
  • Cleanup and review work in the feature/refactor-default-content-type branch. (It should pass once converted to Plack::Test.)
  • Revert fa27fec
  • Raise another issue for implementing send_file(\$data).
@racke
Owner
@xsawyerx
Owner

#552

Here we go. :)

@veryrusty
Owner

@xsawyerx++

I can confirm that #552 plus the work in feature/refactor-default-content-type and a revert of 8171ae8 passes the tests. Go Team!

@veryrusty veryrusty referenced this issue from a commit
@veryrusty veryrusty Remove default_content_type attribute to the Response object.
Moves the logic of what content type to apply by 'default' into the
Response object, resulting in much simplier logic within _dispatch_route.

Big win is that the default content type is not applied until it is actually
required. Ref #550 and #551.
163c4ef
@xsawyerx
Owner
@xsawyerx xsawyerx added Core and removed Pending Review Core-Dev labels
@veryrusty veryrusty referenced this issue from a commit
@veryrusty veryrusty Remove default_content_type attribute to the Response object.
Moves the logic of what content type to apply by 'default' into the
Response object, resulting in much simplier logic within _dispatch_route.

Big win is that the default content type is not applied until it is actually
required. Ref #550 and #551.
e2b5437
@shumphrey
Owner

I'm confused by the status of this ticket.
Current (Dancer 0.12+) send_file is broken in that it ignores the content_type setting.

@veryrusty
Owner

My memory (from 2.5 months ago) is that Pr #555 will resolve the issue.

@xsawyerx commented that there are no tests for send_file( $filename, content_type => $mime_type ); interface that is in the documentation; @shumphrey if you have a use case for this, can you add a test case to #555 ?

@veryrusty veryrusty referenced this issue from a commit
@veryrusty veryrusty Remove default_content_type attribute to the Response object.
Moves the logic of what content type to apply by 'default' into the
Response object, resulting in much simplier logic within _dispatch_route.

Big win is that the default content type is not applied until it is actually
required. Ref #550 and #551.
3b89be0
@veryrusty
Owner

Resolved with the merge of #555.

@veryrusty veryrusty closed this
@xsawyerx
Owner

Are we ready for another release?

@veryrusty
Owner

@xsawyerx yes, please release! :smile:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.