Problem/Motivation

There was a recent security update for the 7.x-dev version for the Location module, and when running "drush up location", it was failing to update that module. The error message was that an md5sum failure occurred for the downloaded file.

I did some research into this, and it looks like there's a problem with the filename assigned to the destination file for the download specifically for -dev projects. In /commands/pm/package_handler/wget.inc, in the function package_handler_download_project(), there is a special case for -dev snapshots, with a comment about adding so that it's part of the cache key. This special case adds "?*timestamp*" onto the end of the download link. The filename is then assigned basename(download link), without also having a special case for dev snapshots.

On Windows, the '?' character cannot be used in a filename, and the copy to the destination file fails as the destination filename ends up being something like 'location-7.x-3.x-dev.tar.gz?date=1346676290'.

Proposed resolution

I'm not certain how dependent other components in drush may be in the use of the '?' in the downloaded/cached filename that gets generated. To ensure that this was specifically the problem that was preventing the update in my case, I added the following after the $filename assignment in the function above, however it would probably be more useful to just convert any '?' into '_' after the filename assignment, at least on Windows, depending on how that might affect other components of drush that might be dependent on the name.

<?php
 
if(strpos($filename, '?') !== FALSE) {
   
$filename = substr($filename, 0, strlen($filename) - strpos($filename, '?'));
  }
?>

Remaining tasks

I'm hoping someone more familiar with other drush components can confirm this is an issue and I'm not overlooking something, and also provide a meaningful solution to the problem that would take into account the dependencies I am not familiar with. Thanks for taking a peek!

Comments

moshe weitzman’s picture

Assigned:Unassigned» dmitry_bezer

Wow, nice research! Your proposed solution sounds reasonable to me. Assigning to Dmitry.

dmitry_bezer’s picture

StatusFileSize
new860 bytes

I would simply do

$filename = str_replace('?', '_', basename($release['download_link']));

I don't see how it can possibly break any existing code...

dmitry_bezer’s picture

[doublepost]

dmitry_bezer’s picture

Status:Active» Needs review
moshe weitzman’s picture

Status:Needs review» Needs work

It looks like drush_download_file() already tries to make a safe filename with $cache_name = str_replace(array(':', '/', '?', '='), '-', $url); and returns the eventual destination. That file is at $path and we call md5_file($path) in our verification. Since $path has already been cleaned of ?, I don't see how this patch matters. I think $filename is only used for printing log messages and not for verification.

Maybe I have something wrong? Would be good to know if this patch actually fixes the problem.

jaminion’s picture

Hey guys, sorry I haven't been able to keep track of this since it was submitted. Thank you very much for the time you've spent reviewing it!

Moshe, I've been looking at the drush_download_file() and it looks like the cleaning that occurs doesn't actually get associated with $destination if the $destination is given, like it is in this particular case. The $cache_name gets set based on the $url rather than the $destination, and at the very end of the block the file from the cache gets copied to the given $destination. That's probably where the error is actually getting generated in my usage. I, uhh, didn't quite catch it ;) Sorry!

Assigning the $cache_name on the path of the $destination to the $destination might work well. If you don't want to change this function much, might also work well to pull the logic in cleaning the $url when setting the $cache_name to a function that could also be used when setting the $filename in package_handler_download_project() too, or just replicate it.

Dmitry, I promise to test your patch in my environments by Monday morning, FWIW. I'm just in a situation now where I can only review things through a browser.

I gotta say, the way you guys are jumping on these reports, I feel bad for having any reservations against supporting Drupal on Windows. And whatever hate I get for admitting that, I deserve!

dmitry_bezer’s picture

Status:Needs work» Needs review

As jaminion said drush_download_file() makes replacement in $url but not in $destination. So when file is copied to $destination - operation fails if path contains '?'. I reproduced the bug and can confirm that replacing '?' -> '_' in package_handler_download_project() fixes the issue.

jaminion’s picture

Hey guys, I can confirm Dmitri's patch works in my cases. Sorry - late again, but thanks!!

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

OK, Dmitry is welcome to commit this or I will if I get to it first.

sketman’s picture

I have the same problem, can not download any dev version of module. Using the latest dev version of 2012-Sep-27.
Has it already been commited there please?

dmitry_bezer’s picture

Status:Reviewed & tested by the community» Fixed

Just committed it.
Sorry for the delay

Status:Fixed» Closed (fixed)

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

joehudson’s picture

thank for the fix!

hansfn’s picture

Status:Closed (fixed)» Active

The fix isn't included in Drush 7.x-5.8 (which was released one month after the commit). I guess the reason is that the fix was commited to the master branch and not 7.x-5.x.

I have created a more general issue for Windows support - see #1844224: Document how to correctly format filesystem paths for Windows shells and insure existing usage is consistent. My patch currently includes the fix from this thread. Not sure what is the best approach. Closing this as duplicate or ...

greg.1.anderson’s picture

Status:Active» Patch (to be ported)

This fix should be ported to the 7.x-5.x branch.

AlmogBaku’s picture

Version:7.x-5.7» 7.x-5.8

Hey!,
It very wired!

I update the code at "C:\ProgramData\Drush\commands\pm\package_handler\wget.inc"
And it seems that it totally ignore the changes!!

I tried to remove and re-adding the %PATH% var.. restarting.. and nothing.
It seems that my Drush ignore the change :(

Somebody have an idea?(I am using windows 8)

hansfn’s picture

And it seems that it totally ignore the changes!!

Cache? Try "drush cc drush".

PS! What is taking so long to just commit this?

AlmogBaku’s picture

thanks, but it doesn't help...

greg.1.anderson’s picture

#17: Testing. You can always speed along bugs that are ready by writing unit tests for them; this will give maintainers more confidence that the patch is ready to be committed. Most of the Drush maintainers use Linux exclusively; more help from Windows users would improve the integration rate.

hansfn’s picture

Hi, greg. The patch for this issue has already been commited to the master/8.x-6.x branch. In addition it's a trivial patch. I just didn't see the need for anymore testing in this case. (I even reported that the patch in this thread fixed this issue on for 7.x-5.8 - ref #1844224: Document how to correctly format filesystem paths for Windows shells and insure existing usage is consistent.)

PS! I Normally exclusively use Drush on Linux too, but I just held a presentation about Drush (on Windows) at the Drupal Camp in Oslo (which lead to 1844224).

greg.1.anderson’s picture

It may look like a trivial patch, but it does change the default filename for downloads, which means that there is some small question as to whether this should be accepted on the stable branch.

I have been meaning to look at #1844224: Document how to correctly format filesystem paths for Windows shells and insure existing usage is consistent for a while now. That issue has no tests, currently.

hansfn’s picture

Status:Patch (to be ported)» Reviewed & tested by the community
StatusFileSize
new567 bytes

So you really wanted a unit test for this - not only RTBC. Anyway, aren't you a little bit too conservative here? We are only changing the default filename for dev downloads. The only problem with the change would be a possible one-time extra download of dev modules since the filename changed. And, a question mark isn't something you link in filename on Linux either since it might require escaping of the question mark to avoid unwanted action in your shell.

Anyway, to stop wasting your and mine time with this discussion, I have attached a patch the only changes the behavior on Windows where downloads of dev modules didn't work at all because of the md5 failure. This patch is only doing good ;-)

greg.1.anderson’s picture

#22 looks safe for backporting to me. Sorry for wasting your time.

hansfn’s picture

Thx for reviewing 22. Then I'll wait for someone to commit it ;-)

PS! The "wasting time" comment came out much harsher than intended. Typically I'm the one wasting my own time because I can't write short replies. Mea Culpa.

greg.1.anderson’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 7.x-5.x. (Also fixed some bugs in Drush iq so that #22 would commit and merge cleanly with drush iq-apply-patch and drush iq-merge.)

Status:Fixed» Closed (fixed)

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

liquidcms’s picture

Title:Windows Drush: dev project update issue with md5 sum failure» dev project update in Windows filesystem fails with md5 sum failure
Version:7.x-5.8» 7.x-5.x-dev
Status:Closed (fixed)» Needs review

re-opening this to make a couple comments:

this fix only "kinda works". i'll change the title to make it more clear. the title incorrectly suggests this is drush windows issue when in fact it is really a windows file naming issue (not a drush issue).

i'll explain my setup to better explain.

my dev system is Win7 (since seriously, who would develop in linux??); but drush does not work very well (still) in Windows. so my solution is that i run an ubuntu vm (drupalpro project) which sees my windows filespace and whenever i need to run a drush command; i simply do it there.

the patch above checks if os is windows that drush is running in. it isn't. but the file space is windows based. so problem persists (and a pita since it also wipes out the files in the module folder that i am trying to update - but i suspect that is a different drush bug than this one).

fyi.. the solution posted here: http://drupal.org/node/1801788#comment-6586138 does work correctly.

hansfn’s picture

Status:Needs review» Active

Hi, liquidcms. You are correct that this is a file system issue so I agree on the title update (FWIW - I'm not a Drush developer). If you look carefully at this thread you'll see that a general fix was suggested, but only a change for Windows was accepted.

PS1! Drush 8.x-6.x-dev works much better on Windows. (I use Drush in MSYS on Windows, but I think it has improved for Windows in general too.)

PS2! I never realized that Linux running in a VM on Windows, would have the limitations of the Windows file system.

liquidcms’s picture

@hansfn, re: PS2 - no that is not the case nor what i am doing. my filesystem that my ubuntu vm sees is my windows filespace (since my devel is done in windows). linux filesystem will work as you'd expect regardless of the VM host (just not what i am doing).

greg.1.anderson’s picture

Version:7.x-5.x-dev» 8.x-6.x-dev
Status:Active» Closed (won't fix)
Issue tags:+needs migration

This issue was marked closed (won't fix) because Drush has moved to Github.

If desired, you may copy this bug to our Github project and then post a link here to the new issue. Please also change the status of this issue to closed (duplicate).

Please ask support questions on Drupal Answers.