Over at #1267228: Drush Make should use Drush core's native download abilities concurrently, _make_download_file() is causing some problems for concurrency. It's also lots of duplicated code.

CommentFileSizeAuthor
#6 drush-replace-make-download-file-1404416-06.patch7.99 KBjhedstrom
#5 drush-replace-make-download-file-1404416-03.patch7.35 KBjhedstrom
#1 drush-replace-make-download-file-1404416-01.patch6.31 KBjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Active » Needs work
FileSize
drush-replace-make-download-file-1404416-01.patch6.31 KB

This patch is a start, but one test is failing since _make_download_file was doing quite a bit of magic with response headers for manipulating downloaded files.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs work » Needs review

The test that is failing is the one that uses POST to download. Moshe thinks this could safely be removed, and I personally haven't used POST in a make file that I remember, so we may be able to remove that functionality.

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented
Status: Needs review » Needs work

Does that mean that _make_download_file was actually better? Should we be replacing the other way around?

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented
Status: Needs work » Needs review
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
FileSize
drush-replace-make-download-file-1404416-03.patch7.35 KB

This patch removes the POST functionality, and all tests are passing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
FileSize
drush-replace-make-download-file-1404416-06.patch7.99 KB

This patch turns caching on, and removes the make_download_post() function.

As msonnabaum suggested in IRC, somebody can always re-implement POST in contrib if it's really needed (see Make CVS for an example).

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Great. Could we try to let drush_download_file() put the file in its $destination? If not possible, use drush_move_dir().

IMO, this is RTBC and should be committed now as this is crucial for a drush5 release.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Fixed

Fixed up per #7 and committed since tests are passing. Lets get that concurrency patch in!

Am creating a Change Notification record for this now.

helmo’s picture

helmo CreditAttribution: helmo commented

Well I'm someone who needs the POST support. I need it to download tinymce translations.

I've spent some time to create a make_post project as suggested in #6

See: http://drupal.org/project/make_post

I'd love to welcome an extra maintainer there...

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David Stosik’s picture

David Stosik CreditAttribution: David Stosik commented
Title: Replace _make_download_file with drush_download_file » Replace _make_download_file with drush_download_file update drush make documentation
Status: Closed (fixed) » Active

The file commands/make/README.txt still mentions this

`request_type` - the request type - get or post. Defaults to get. Optional.

`data` - The post data to be submitted with the request. Should be a
valid URL query string. Required.

Shouldn't this be removed, as we need an extra module to generate POST requests ?

David

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Active » Fixed

Updated docs to point to new module.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.