Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#10 | 700558-10.update_manager_file_extensions_test.patch | 887 bytes | Scott Falconer |
#6 | 700558-6.update_manager_file_extensions_test.patch | 2.49 KB | dww |
#2 | 700558-2.update_manager_merge_file_extensions.patch | 3.21 KB | dww |
#1 | 700558-1.update_manager_merge_file_extensions.patch | 726 bytes | dww |
Comments
Comment #1
dwwComment #2
dwwNow 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.
Comment #3
Dries CreditAttribution: Dries commentedThis looks good to me. For a while, I was confused about 'update_test_archive' being the actual extension.
Comment #4
dwwBased 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!
Comment #5
webchickI 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:
...need a line of PHPDoc.
Comment #6
dwwThe 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.
Comment #7
chx CreditAttribution: chx commentedAdds a test which passes. So why not :) ?
Comment #8
dwwDries committed this... Yay.
Comment #10
Scott Falconer CreditAttribution: Scott Falconer commented700558-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():
Patch below changes the test to check for 'tar', which will always be displayed.
Comment #11
jersu CreditAttribution: jersu commentedgood catch. fixes fragile test - 'zip' is only displayed conditionally, but 'tar' is always an available option as per system_archiver_info().
Comment #12
webchickCommitted to HEAD. Thanks!