While working on a patch for #693082: Add an ArchiverZip class to support .zip files in update manager I noticed that update.manager.inc doesn't correctly merge all possible file extensions since it's using += instead of array_merge(). Trivial fix, stay tuned.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Status: Active » Needs review
FileSize
726 bytes
dww’s picture

Now with a test. ;) I added a whole new test class for this, since we need an admin user with 'administer software updates' to see the admin/modules/install page. We're going to need more tests for this page, anyway (e.g. once the dust settles from #693084: Regression: file_munge_filename() extension handling broken by move to File Field). Confirmed that the test fails if you reverse apply #1.

Dries’s picture

This looks good to me. For a while, I was confused about 'update_test_archive' being the actual extension.

dww’s picture

Status: Needs review » Reviewed & tested by the community

Based on bot happiness in #2 and Dries's review in #3, bumping this to RTBC. #693082: Add an ArchiverZip class to support .zip files in update manager is in so this is a visible bug already in core, not just when contrib tries to extend this. Let's get this in. Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I interpret Dries's review as "needs work", in that we should make the bogus file extension sound more file extensiony. I agree that it wasn't at all obvious reading the test. How about just "foo", with a clarifying comment above that mentions this is a bogus extension for testing purposes?

On a more minor note, these:

+class UpdateInstallTestCase extends DrupalWebTestCase {
...
+  function testFileNameExtensionMerging() {

...need a line of PHPDoc.

dww’s picture

Title: Update manager doesn't properly merge supported filename extensions » Add test to ensure filename extensions are properly merged in the Update manager UI
Category: bug » task
Status: Needs work » Needs review
FileSize
2.49 KB

The underlying bug was fixed over at #747252: Cannot extract themes and modules although there was no test added for this specific bug. Rerolled the patch to just add the test.
- Added PHPDoc per webchick #5
- Renamed the bogus extension to 'update-test-extension'. I hope that's self-documenting enough. I'd rather not just call it 'foo' as that seems both less obvious and more fragile.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Adds a test which passes. So why not :) ?

dww’s picture

Status: Reviewed & tested by the community » Fixed

Dries committed this... Yay.

Status: Fixed » Closed (fixed)

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

Scott Falconer’s picture

Category: task » bug
Status: Closed (fixed) » Needs review
FileSize
887 bytes

700558-6.update_manager_file_extensions_test.patch checks for 'zip', but 'zip' won't be found if zip_open() isn't available per system_archiver_info():

function system_archiver_info() {
  $archivers['tar'] = array(
    'class' => 'ArchiverTar',
    'extensions' => array('tar', 'tgz', 'tar.gz', 'tar.bz2'),
  );
  if (function_exists('zip_open')) {
    $archivers['zip'] = array(
      'class' => 'ArchiverZip',
      'extensions' => array('zip'),
    );
  }
  return $archivers;
}

Patch below changes the test to check for 'tar', which will always be displayed.

jersu’s picture

Status: Needs review » Reviewed & tested by the community

good catch. fixes fragile test - 'zip' is only displayed conditionally, but 'tar' is always an available option as per system_archiver_info().

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix

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