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.Both Dries (via IRC) and webchick (via #538660-154: Move update manager upgrade process into new authorize.php file (and make it actually work)) pointed out that we need much better validation and error reporting for the text box where you're supposed to enter the URL for a release archive file you want to download and install on your site.
If you give it a bogus string, you get a cryptic PHP notice:
Notice: Undefined index: scheme in update_manager_file_get()
If you give it a URL that doesn't point to a tarball, you end up getting a cryptic exception from Archive_Tar. :(
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | um_validation-606180-1.patch | 819 bytes | JacobSingh |
Comments
Comment #1
JacobSingh CreditAttribution: JacobSingh commentedOkay, this led me on a bit of a wild goose chase and made me create / update 3-4 issues.
This patch is pretty small in comparison.
The main ones which need immediate attention are:
#604618: Create a common interface for Archive operations so we can handle .zip, .tar.gz.
(http://drupal.org/node/604618#comment-2179086)
and
#605918: Port update manager to use the new Archiver class, not directly Archive_Tar
(http://drupal.org/node/605918#comment-2179118)
Here is what happens when I enter various URLs now:
ashdfuashaiuashfdisfuhasf
The provided URL is invalid.
smb://windows/file.tar.gz
The provided URL is invalid. This is because of:
#611466: valid_url doesn't return urls which are not http or ftp
http://drupal.org/index.php
w/o my patches, this returns " Invalid checksum for file "" "
w/ my patches:
Cannot extract /Users/jacob/work/github/drupal7/sites/default/private/temp/update-cache/index.php, not a valid archive.
http://ftp.drupal.org/files/projects/IAMA404
http://drupal.org/IAMA404 could not be saved.
Unable to retreive Drupal project from http://drupal.org/IAMA404.
This is not so bad, but I filed this anyway:
#611542: system_retrieve_file should not set messages, or be split up so it can be used for API calls
Comment #2
JacobSingh CreditAttribution: JacobSingh commentedComment #3
Bojhan CreditAttribution: Bojhan commentedhttp://drupal.org/IAMA404 could not be saved. - What does that mean?
Comment #4
jim0203 CreditAttribution: jim0203 commentedIn and of itself, this patch traps strings that are not valid URLs and works fine - pass. Not sure how it works with the the other patches, but it's fine on its own.
Comment #5
dww@Bojhan: That's exactly what #611542: system_retrieve_file should not set messages, or be split up so it can be used for API calls tries to fix. There's no way to improve that error message without fixing the function that we're calling to attempt to download the file.
Anyway, #1 is a step in the right direction, does what it says, the code is clean, bot is happy. RTBC.
Comment #6
dwwp.s. @JacobSingh: If you want to refer to a specific comment in an issue, just add a dash and the comment number after the
[#nid]stuff. Like so.#604618-23: Create a common interface for Archive operations so we can handle .zip, .tar.gz.gives you:#604618-23: Create a common interface for Archive operations so we can handle .zip, .tar.gz.
which takes you directly to comment #23 on issue #604618...
Comment #7
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #8
dwwFor those following along closely at home, http://drupal.org/cvs?commit=279222 contains both this patch and the bulk of the changes for #605918: Port update manager to use the new Archiver class, not directly Archive_Tar (all the fixes for update.manager.inc). So, CVS annotate is going to be confusing and weird, but at least all the code is in core so nothing's actually broken.
Comment #9
dwwSlightly whacky follow-up idea: #614300: Support d.o release node URLs when installing new code, not just links to .tar.gz files directly