Posted by dww on January 31, 2010 at 6:07pm
7 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | update.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | dww |
| Status: | closed (fixed) |
| Issue tags: | Quick fix |
Issue Summary
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.
Comments
#1
#2
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.
#3
This looks good to me. For a while, I was confused about 'update_test_archive' being the actual extension.
#4
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!
#5
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.
#6
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.
#7
Adds a test which passes. So why not :) ?
#8
Dries committed this... Yay.
#9
Automatically closed -- issue fixed for 2 weeks with no activity.
#10
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():
<?phpfunction 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.
#11
good catch. fixes fragile test - 'zip' is only displayed conditionally, but 'tar' is always an available option as per system_archiver_info().
#12
Committed to HEAD. Thanks!
#13
Automatically closed -- issue fixed for 2 weeks with no activity.