Install module from .zip URL fails

This is happening to my site also but only on my local server. When I try to install via a tar.gz ( ie. https://ftp.drupal.org/files/projects/captcha-8.x-1.0-beta4.tar.gz) it gives me a white screen at this URL:

http://DOMAIN/core/authorize.php?batch=1&id=160&op=start

Why I try the .zip format it gives me:

"Cannot open temporary://update-cache-1559f6ff/captcha-8.x-1.0-beta4.zip"

I'm facing the same problem when I have updated to version 8.8 using .zip link. But when I install using .tar.gz link it works normally.

For me the same problem.
When my site was updated to 8.8.0 and later 8.8.1 I could no more upload a new theme.
Error received: " Cannot open <em class="placeholder">temporary://newtheme.zip</em> "
With earlier versions of drupal I never had this problem.
Upload place: home>>admin>>appearance>>list>>install new theme>> newtheme.zip>> install -> error

Thanks.

I experienced the same problem.

Following your guide #3, I unzip the module and compress it as .tar.gz. Then install as normal, it works fine.

Most welcome @tuthanh, hope #3 works for all.

I'm experiencing the same problem on 8.8.1

Is .zip no longer supported?

I agree .tar.gz is an option and possibly a workaround but I wouldn't consider this issue fixed if .zip isn't working.

I have the same problem, I am testing locally upgrading from drupal 8.6 to 8.8.1
and I confirm also when unzip the theme and compress it as tar.gz and also even as .tar only
it works.

I have the same problem on 8.8.1

i have the same problem in my localhost(xamp server) on windows 10(2020) with drupal 8.8.1
(Apache/2.4.41 (Win64) OpenSSL/1.1.1c PHP/7.3.12)

████████████████████████████████████████████████████████
the solutions is :

just use this format type (((((( .tar.gz )))))) to solve this problem.

████████████████████████████████████████████████████████

The issue is that module installation via zips doesn't work - using .tar.gz files is a workaround, it isn't a solution, so i don't think we can claim this is "fixed"?

+1 same here..

Yes same issue as of 2/28/2020 at 6pm for my server (haven't tried on localhost). Installing from composer doesn't work either, it installs successfully but no downloaded modules via composer shows up on my drupal install.

Using tar.gz file works but is a pain to do it as i have all zip files already stored on my computer that i upload to each project.

Confirmed this locally. Definitely a bug, not a support request. Major, by these standards:

Bugs that have significant repercussions but do not render the whole system unusable (or have a known workaround) are marked major.

8.8.x Works fine if I use a .tar.gz URL, but fails with a .zip URL.
8.7.x and even 7.x work fine with both .tar.gz and .zip archives.

Upon closer inspection, this was broken by:

#2908176: Deprecate archiver procedural functions

That's what changed between 8.7.x and 8.8.x that causes this to fail.

\ZipArchive::open() can't handle file streams like temporary://update-cache-f1a62f70/google_analytics-8.x-2.4.zip

The "deprecated" code does this:

function archiver_get_archiver($file) {
  // Archivers can only work on local paths
  $filepath = \Drupal::service('file_system')->realpath($file);
  if (!is_file($filepath)) {
    throw new Exception(t('Archivers can only operate on local files: %file not supported', ['%file' => $file]));
  }
  return \Drupal::service('plugin.manager.archiver')->getInstance(['filepath' => $filepath]);
}

In 8.8.x, this has been replaced with:

$archiver = \Drupal::service('plugin.manager.archiver')->getInstance([
    'filepath' => $file,
]);

So, we completely lost the comment that archivers only work on local paths, and the call to realpath(). :(

This is the smallest possible patch that gets it working again locally.

Tagging for needing tests, since the lack of test coverage for installing from .zip files is what allowed #2908176: Deprecate archiver procedural functions to land as it did.

Working on tests now. Stay tuned.

Wow, trying to get these tests to fail is a total WTF. :( For reasons that absolutely escape me, the attached tests pass locally. I simply cannot explain this. On a whim, let's see what the d.o test bot does with this.

Wow, thank god! I dunno WTF is going on with phpunit on my local install. :(

Okay, so let's try again, with a more valid test-only (hopefully fails without #16), and a full patch including the fix from #16.

Still has changes to drupalci.yml to save bot cycles while this is sorted out.

Finally figured out WTF was going on with phpunit locally... I had a second copy of core/modules/update in my filesystem, left over from other random crap I was doing in the workspace. 🤦‍♂️So somehow the main copy I was hacking was partly being used (e.g. for the data provider and all that), but the other copy was the actual version of FileTransferAuthorizeFormTest being tested with. Argh.

Anyway, since #20 failed exactly as expected, and since I can now reliably run this locally, here's a slight cleanup of the test cases, a new test-only (that still uses drupalci.yml to only run itself) and a full patch that might be RTBC. Also attaching interdiff between the two. This is the first truly legit patch worth reviewing, so I'm not including an interdiff from any of the previous efforts.

The fix looks great and it's good to have test coverage.

I've unzipped the files and confirm they only contain update_test_new_module/update_test_new_module.module and update_test_new_module/update_test_new_module.info.yml however both zip files contain core: 8.x - this needs to be removed so the tests work on 9.0.x

We could, but it doesn't actually matter. These tests don't enable those modules. The final assertions are:

    // Ensure the module is available to install.
    $this->drupalGet('admin/modules');
    $this->assertText('Update test new module');

This test class is only testing that the download/install stuff "works". Obviously we're not testing remote fetching. Nor the ftp/ssh stuff (there's mockery all over the place). But at least it shows that we can't open a .zip file in 8.8...

So yeah, we could re-generate the .zip files as if these dummy modules could be installed on D9, but that seems out of scope, since the tests don't do that at all. /shrug

In other news... are we really happy with fixing this here directly in update_manager_archive_extract()?

archiver_get_archiver() was deprecated in D8.
It used to handle the realpath() for you.
It was replaced with a slick new service.
The slick new service makes the caller worry about this.
Callers can't be trusted. ;)

Seems yucky. I'd kinda rather fix the slick new service to give you a working Archiver, just like archiver_get_archiver() used to do.

That's a differently-scoped fix/change, but that seems more friendly to anyone migrating to 8.8 and above...

Thoughts?

Thanks!
-Derek

For the record, I made the .zip files with:

tar -zxf update_test_new_module.tar.gz
zip -r update_test_new_module.zip update_test_new_module

That's why I think changing the contents is out of scope.

Heh, except in the 9.0.x branch the .tar.gz files were changed to include this:

core_version_requirement: '*'

So here's a 9.0.x-appropriate version of the patch. ;)

Also including new test-only to fail, and a 3101299-28.9_0_x.only-run-new-test-with-fix.patch that includes the fix plus drupalci.yml to only run the new test. I'll queue that one on all the branches to see if a single commit can be cherrypicked back to 8.8.x, or if we should commit different versions.

core_version_requirement: '*' is no longer necessary either :) we fixed it so that test modules don't have to declare any core compatibility as it is just noise.

Also this code is all the way back to D8.8 so there's no need for separate 8.8, 8.9 and 9.0 patches - they can all be the same patch.

Hah. :)

Well, it seems weird to have the contents of the .zip and .tgz diverge. And it seems like scope creep to change the contents of the .tgz in this issue.

Shall we go forward with #28 + #22 and follow-up to remove from both?

Or do we expand scope?

Also, thoughts on #26?

Thanks,
-Derek

I don't think it matters about the contents of the zip and tgz being different. I tried to get the entire remove all the core: stuff from tests backported earlier but it hasn't happened.

I think the more important consideration here is whether we're fixing this in the right place. Looking at \Drupal\Core\Archiver\ArchiverManager::getInstance() we're already doing stuff there to select the correct plugin using the filepath so it's not a big stretch to make it a real path.

Okay, fixed the .zip contents an leaving .tar.gz as-is.

I'd prefer fixing it in \Drupal\Core\Archiver\ArchiverManager::getInstance(), too. How's this?

Do we need the optional constructor argument dance? If so, what should the deprecation message look like? 3101299-33.fix-archivermanager-with-deprecation.patch ?

Should we keep the .tgz in sync, too? 3101299-33.fix-archivermanager-with-deprecation-fix-tgz.patch

Uploaded the last 2 as do-not-test, but I guess we can queue them if we're interested...

Re: #34: https://www.drupal.org/pift-ci-job/1597518 is a random fail from Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest
#3103492: Random fail in WidgetUploadTest
But it's not worth re-queuing since hopefully we'll go with #33 or something like it.

p.s. if we're keeping the deprecation:

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.php
@@ -26,11 +34,18 @@ class ArchiverManager extends DefaultPluginManager {
+      @trigger_error('Not defining the final file_system argument to ' . __METHOD__ . ' is deprecated in drupal:9.0.0 and will throw an error in drupal:10.0.0. See https://www.drupal.org/node/2999951', E_USER_DEPRECATED);

This is pointing to the CR from #2908176: Deprecate archiver procedural functions. Do we want a separate CR for this constructor change, even though it was only available in a few releases in the wild since it was introduced? Or should we mention the constructor change at the old CR and be done?

@dww I wouldn't bother with a CR for the constructor - they are not part of the BC promise - explicitly.

I think we should make the deprecation since 8.8.x since we want to fix the bug there. Even though we're changing a service .yml because we have the BC layer in everything will be fine.

Okay, sounds good. Like so? interdiff is relative to 3101299-33.fix-archivermanager-with-deprecation.patch

Anything else?

Thanks,
-Derek

I pinged @catch about the deprecation in Drupal 8.8 and he said

Seems OK for backport yeah.

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.php
@@ -26,11 +34,18 @@ class ArchiverManager extends DefaultPluginManager {
+      @trigger_error('Not defining the final $file_system argument to ' . __METHOD__ . ' is deprecated in drupal:8.8.0 and will throw an error in drupal:10.0.0.', E_USER_DEPRECATED);

Should be 8.8.3 (hopefully).

Fixed #39

I don't think it's a best-practice to use data-providers with functional tests, but because of the usecase here I think it is better to do it like this instead of adding a complete copy of that testcase for the zip.

The test covers the added code, as expected, and the actual fix looks solid as well.

Committed and pushed de27c163f0 to 9.0.x and 75ada80424 to 8.9.x and 711decb5c2 to 8.8.x. Thanks!

Backported to 8.8.x after discussing with @catch.

@borisson_ re: #41: Yeah, I totally agree. I meant to comment about that. I spent a little while trying to get this working in a single test case, but it was turning into a convoluted mess. Seemed way cleaner and better to just use a dataProvider here, even though I definitely agree that's often not a good choice for Functional tests.

@alexpott: Thanks for landing this in time for 8.8.3!

Cheers,
-Derek

This is still a problem in the very latest version of D9.

It wouldn't be an issue if only tar.gz and not zip files were provided by contributors or vendors.

Hint..... !

I just wasted half an hour with a theme sorting this out. Grrrr.