On irc dww was talking about #1006238: Add a weight limit for project_release_handler_field_files. As I tested zip last year and it failed, I was curious if it worked today. It didn't...

This doesn't seem a big issue as every project is packed as .tar.gz on drupal.org. But apparantly we are going to add the possibility to download the project as a zip file. Every sane windows user will download a zip and not the tar.gz. Those people will complain cause the installation fails which leads to dissapointed (maybe new?) drupal users.

We have to fix this before drupal gets released, BUT I agree it's not critical.

Here is the error message as a starting point:
UpdaterException: Unable to determine the type of the source directory. in Updater::factory() (line 100 of ...\includes\updater.inc).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Component: update system » update.module

Re component: see #990366: Rename core's "update system" component to "database update system" ... ;)

Yeah, that sucks. We tried to make this work. I can confirm it doesn't work, even with a "properly" packaged .zip file on d.o itself:

http://drupal.org/dww/apachesolr-7.x-1.x-dev.zip

I don't have time to debug this now, but it's a real bug, and we should *really* try to fix this before 7.0, since .zip is hopefully going to be live by then.

aspilicious’s picture

This is the problem funtion:

function update_manager_archive_extract($file, $directory) {
  $archiver = archiver_get_archiver($file);
  if (!$archiver) {
    throw new Exception(t('Cannot extract %file, not a valid archive.', array ('%file' => $file)));
  }

  // Remove the directory if it exists, otherwise it might contain a mixture of
  // old files mixed with the new files (e.g. in cases where files were removed
  // from a later release).
  $files = $archiver->listContents();
  // Unfortunately, we can only use the directory name for this. :(
  $project = drupal_substr($files[0], 0, -1);
  $extract_location = $directory . '/' . $project;
  if (file_exists($extract_location)) {
    file_unmanaged_delete_recursive($extract_location);
  }

  $archiver->extract($directory);
  
  return $archiver;
}

Solution:

Replace
$archiver->extract($directory);
with
$archiver->extract($directory, $files);

Zip archiver need the array of files, tar archiver don't :)

There are some flickering errors but it installs ok!

aspilicious’s picture

Status: Needs review » Active

*ignore this*

aspilicious’s picture

I got the following warnings. http://awesomescreenshot.com/08552aef1

*I think* the problem is that the files have forward slashes in it, that's something the stat function doesn't like.
But I don't understand the "no active batch".

bfroehle’s picture

Status: Active » Needs review
FileSize
847 bytes
522 bytes

Here's a patch which should address the original issue. The idea for fixing it was copied from the similar treatment in the tar implementation.

I spent a good amount of time tracking down the warnings which appear in #4. The backtrace for each warning shows that it is coming from ZipArchive::extractTo. In each case, stat is being called to determine if a directory exists. (In each case, the directory does not exist). What I think is happening is that the PHP Zip library is failing to set STREAM_URL_STAT_QUIET internally. I think the attached patch for PHP 5.3.4 (!!) would probably fix the problem. I don't have time to test it at the moment. I'll file a bug report with PHP if it turns out we think this is the cause of the problem.

Edit: PHP bug report filed at http://bugs.php.net/bug.php?id=53603.

aspilicious’s picture

Status: Active » Needs review

As you can see I was wrong, it doesn't need $files :) (looked at some examples)
==> Learned a lot :)

Thnx for looking into this!

aspilicious’s picture

Those php dudes answered:

Fixed in SVN, but you should probably also fix the url_stat of the stream wrapper to not emit messages when a file doesn't exist (since this is the behavior with the plain wrapper).

bfroehle’s picture

FileSize
1.31 KB

This is #5 + fixing url_stat to not emit messages when a file doesn't exist.

hass’s picture

+

bfroehle’s picture

The patch in #8 allows me to install from zip files and doesn't flash a screen of error messages. Anybody else care to comment?

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

I installed 10 modules with this patch, works perfect, only affects zip archive.
Same code as tar archive which is working perfectly and this code can't break anything.

More details for webchick:
-------------------------
- Without this patch the installation of zip archived modules stops somewhere in the middle.
- This is caused by and empty file array that we are passing to the extracter
- Patch copies the workflow of the tar extracter.

- The other change fixes url_stat to not emit messages when a file doesn't exist. This has to be done due to a phpbug.
(see bug report http://bugs.php.net/bug.php?id=53603)

Result: working installer + no anoying warnings/errors

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work on this, folks.

Though this could ideally use dww's sign-off, this is a pretty nasty bug. I think it's better to fix this before release and then go back to clean it up if necessary. aspilicious has done ample manual testing to ensure it's working properly.

Committed to HEAD. Thanks!

webchick’s picture

Oh and an EXTRA bfroehle++ for fixing upstream PHP bugs as part of Drupal patches! :D Awesome!!

dww’s picture

Thanks for fixing this, everyone! Sorry I haven't been able to comment. Not only have I been slammed with tons of work, and a nasty sore throat, the net where I'm currently staying utterly sucks and I can only seem to be online for about 10 minutes at a time... :( But yeah, patch looks fine, and yes, way to go finding and patching upstream PHP bugs while we're at it. ;)

Cheers,
-Derek

Status: Fixed » Closed (fixed)

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