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

Already on GitHub? Sign in to your account

Lib archive filename should be ascertained from Content-Disposition response header when such a header exists #27

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

rsimoes commented Feb 21, 2013

Consider the following URL:

http://nodeload.github.com/yaml/libyaml/tar.gz/master

When Alien::Base::ModuleBuild downloads from this location, the file is saved as "master." Archive::Extract is then used on the file, but it's not able to determine the file type and croaks. To make Archive::Extract happy, the file name should be what is stipulated in the resource's Content-Disposition response header:

Content-Disposition:attachment; filename=libyaml-master.tar.gz

I may or may not get a chance to write up a patch tonight but figured I'd log the issue either way.

Owner

jberger commented Feb 20, 2013

Thanks @rsimoes. I would happily accept a patch, otherwise I will leave this open to remind me.

I threaten every now and again to totally rewrite the ::HTTP class. It was a halfhearted effort. FTP (being lower hanging fruit and also how GSL is distributed) has been my focus.

rsimoes commented Feb 21, 2013

For lack of adding a full HTTP test to Alien-Base, I manually tested with my Alien-LibYAML module using the the archive URL above.

Owner

jberger commented Feb 21, 2013

I like the idea of using dependent libraries as unofficial tests of AB. It is the most "correct" path of testing that is really open to a library as abstract as AB is. I do this too, using Acme:: modules. Could you perhaps add a quick note to Alien::Base::Authoring.pod/EXAMPLES noting that the your YAML module serves as supplemental testing for HTTP delivery? After that I'm ok with merging.

And thanks!

Owner

jberger commented Dec 15, 2013

Sorry for the long lapse on this. I'm trying to get back to AB maintenance as possible.

I have a new question about this patch. It does not fully implement the content-disposition spec specifically the filename="quotedstring" handling. I have been looking for a nice content-disposition parser on CPAN but haven't found one. Text::Balanced may be needed.

Contributor

mohawk2 commented Aug 30, 2014

@rsimoes, do you have the opportunity to look into this? I'd like to get this closed off.

Owner

plicease commented Aug 31, 2014

How about just change the regex to /filename=("?)(.+)\1/ or maybe /filename=("?)([^"]+)\1/. It doesn't exhaustively handle all corner cases, but does it need to?

@plicease plicease referenced this pull request Sep 3, 2014

Merged

content-disposition support #55

Owner

plicease commented Sep 4, 2014

@rsimoes I took your branch and made some tweaks and merged your changes. You can see #55 for details.

@plicease plicease closed this Sep 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment