Skip to content
This repository has been archived by the owner on Mar 7, 2019. It is now read-only.

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

rsimoes
Copy link

@rsimoes 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.

@jberger
Copy link
Member

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
Copy link
Author

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.

@jberger
Copy link
Member

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!

@jberger
Copy link
Member

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.

@mohawk2
Copy link
Contributor

mohawk2 commented Aug 30, 2014

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

@plicease
Copy link
Contributor

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
Copy link
Contributor

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants