If a user doesn't have permission to write to the main Drupal directory, then pm-download fails. This happens even when --destination or --use-site-dir are used.

This is occurring because function package_handler_download_project in pm/package_handler/wget.inc is using the current working directory for the wget and curl calls, even though drush_pm_download in commands/pm/pm.drush.inc specifies that it should use a temporary directory.

This was a hard issue to diagnose because all of the debug output claims the temporary directory is being used, but the actual wget and curl calls ignore it.

Instead of changing all the wget, curl, and validation calls, my proposed fix just changes the current working directory at the start of package_handler_download, and then restores it to the previous value at the end of the function.

Issue #1168812: Respect filesystem permissions suggests that a temporary directory shouldn't be used, or at least that the temporary directory should live under the sites directory. This patch should not interfere with any of those approaches, so long as they set $request['base_project_path'] appropriately before the call to package_handler_download.

CommentFileSizeAuthor
#12 drush-1272820.patch1.88 KBjonhattan
#8 pm_download_use_tmp_dir-1272820-8.patch1.43 KBresplin
#1 pm_download_use_tmp_dir-1272820-1.patch842 bytesresplin

Comments

resplin’s picture

resplin commented
StatusFileSize
new pm_download_use_tmp_dir-1272820-1.patch842 bytes

Patch attached.

resplin’s picture

resplin commented
Assigned: resplin » Unassigned

I forgot to mention that I verified the bug exists in version 4.5 and version 4.6-dev checked out from master.

The patch applies cleanly to both versions and solves the problem in both versions.

greg.1.anderson’s picture

greg.1.anderson commented

See also #1168812: Respect filesystem permissions. It's one or the other...

resplin’s picture

resplin commented

As I mentioned in the body of the issue, I don't think it is one or the other.

The current functionality is broken in that what happens is not what the code comments and debug messages say should happen. This patch brings behavior in line with the expectations within the code.

Should #1168812 be implemented, this patch will still be necessary.

greg.1.anderson’s picture

greg.1.anderson commented
Title: pm-download isn't using the tmp directory » pm-download isn't using the directory stipulated in $request['base_project_path']

Missed the last paragraph of #0. Here's a better title; please correct if I got it wrong.

steinmb’s picture

steinmb commented

Also got confused this where, drush wrongly claim to be downloading to temp. Dunno what's the best solution, dl to temp, or dl to root (currently in 4.5), but at least should the --debug info. from drush be correct.

resplin’s picture

resplin commented

greg.1.anderson: Thank you for the improved title.

steinmb: Downloading to Drupal root doesn't work for environments that don't have permission there. It needs to honor the --use-site-dir flag.

resplin’s picture

resplin commented
StatusFileSize
new pm_download_use_tmp_dir-1272820-8.patch1.43 KB

I found another instance of the same problem when wget and curl are called in includes/drush.inc in order to download the release history. The 7.x-4.5 release tries to use the Drupal directory by default, which often doesn't have write permission in a multi-site install. When applied to the 7.x-4.5 tarball, the attached patch allows downloads to succeed by fixing both the permission problem in includes/drush.inc, but also in commands/pm/package_handler/wget.inc (it includes my previous patch).

However, this patch does not apply cleanly against the current 7.x-4.x branch (reports as 4.6-dev). _drush_download_file was refactored, but appears to suffer from the same permissions problem. In that branch, commands/pm/package_handle/wget.inc definitely needs to be patched to work. But something else is also getting hung up when the Drupal directory is not writeable.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs review » Needs work

This patch is only for the 4.x branch; in master, _drush_download_file is downloading to the location drush_tempnam('download_file') stipulates; it is then moved to $destination, which is 'base_project_path'.

If we're going to fix this in 4.x, seems like we should backport the code that exists in master rather than doing it some other way. In any event, the submitted patch should be against the 7.x-4.x branch.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Assigned: Unassigned » jonhattan

#8 was fixed in #1273342: Download release history to tmp instead of drupal root.

Let me analyze the OP issue. I'm all for downloading to tmp folder always.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Version: 7.x-4.5 » All-versions-4.x-dev

It is not an issue in drush 5.x, that uses drush_download_file() --that was buggy and fixed in #1273342

In 4.x it is a regression introduced in http://drupalcode.org/project/drush.git/commitdiff/2f28b7f244225a1db5306...

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Assigned: jonhattan » msonnabaum
Priority: Major » Critical
Status: Needs work » Needs review
StatusFileSize
new drush-1272820.patch1.88 KB

Attached patch fixes the regression (untested). The other option is to leverage drush_download_file().

It is Mark's decision what way to go.

anarcat’s picture

anarcat commented
Status: Needs review » Reviewed & tested by the community

this works. I can commit it if marks doesn't veto it. this is a significant regression in Aegir environments.

greg.1.anderson’s picture

greg.1.anderson commented

I agree that it would be a good thing for this to go into 4.x.

msonnabaum’s picture

msonnabaum commented

I'm fine with this.

anarcat’s picture

anarcat commented
Status: Reviewed & tested by the community » Fixed

patch pushed after double-checking with mark on irc

anarcat’s picture

anarcat commented
Status: Fixed » Needs work

Hold the 4.x release presses! :) This creates *another* regression:

chdir(): No such file or directory (errno 2) drush.inc:1216         
[warning]
Extracting masquerade-7.x-1.0-rc4.tar.gz failed, as the destination  [error]
directory  was not found or could not be written to.
Source directory /tmp/drush_tmp_1331850192/masquerade is not readable[error]
or does not exist.
Project masquerade (7.x-1.0-rc4) could not be downloaded to          [error]
/var/aegir/platforms/drupal-7.12.2-build-2012.03.01-staging/sites/gboudrias-d7.site.koumbit.net/modules/masquerade.

weird...

I have reverted the previous patch, back to the drawing board - all of a sudden, backporting drush_download_file seems interesting again.

msonnabaum’s picture

msonnabaum commented

The drush_download_file() backport is certainly my preference, but since we're so close to the drush 5 release, I'm ok with anything that doesn't create a regression.

greg.1.anderson’s picture

greg.1.anderson commented

Sorry I didn't have time to work on this today (and therefore won't be getting to it). I did take a look at it, though, and noticed that drush_download_file has already been ported to Drush 4; it's right there in includes/drush.inc. The code in wget.inc is similar enough that it should be relatively straightforward to move the code from Drush 5 over to Drush 4. Or so it appears.

greg.1.anderson’s picture

greg.1.anderson commented
Version: All-versions-4.x-dev » 8.x-6.x-dev
Status: Needs work » 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.