Page MenuHomePhabricator

Improve error reporting for uploads via URL
Closed, ResolvedPublic

Description

Author: nickpj

Description:
The error reporting on URL upload (via the $mUploadError member variable of the
UploadForm class in includes/SpecialUpload.php) can give an unrelated error
explanation. The error the user sees is: "This file is bigger than the server is
configured to allow." However the possible explanations are:

A) The URL string did not start with "HTTP://" or "FTP://" (e.g. could be

"HTTPS://")

B) You don't have permission to upload files this way.
C) Could not open a temporary file.
D) There was a CURL-related error retrieving the file (e.g. the website

specified was down).

E) The max upload size was exceeded.

Only cases B) and E) are handled currently, and all the others will show the
unrelated error message for case E).

To reproduce:

  1. Add this to LocalSettings.php: -------------------

$wgAllowCopyUploads = true;

$wgGroupPermissions['*']['upload_by_url'] = true;

  1. Go to [[Special:Upload]], select the radio button for a URL, and enter a URL

such as: "https://www.ibm.com/i/v14/t/ibm-logo.gif", and click "Upload file",
and observe the error message.


Version: unspecified
Severity: enhancement

Details

Reference
bz7820

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 9:22 PM
bzimport set Reference to bz7820.

ayg wrote:

Proposed patch, partially tested

I can't fully test this because I don't have curl enabled on my local PHP copy,
but it should work fine.

attachment 7820.patch ignored as obsolete

nickpj wrote:

diff against 17472 which just adds the trivial updates mentioned below to your patch

Your patch looks good and works great! I've tested it and made a very trivial
update (using $errornum instead of the boolean $error).

Only other very small things to do with tweaking the text of error messages
are:

  • For 'upload-proto-error-text', maybe instead of:

"Remote uploads may only take on URLs beginning with http:// or ftp://."
this may be very marginally slightly better:
"Remote upload requires URLs beginning with http:// or ftp://."

  • You may not need error 3 or error 7 - I tried some completely invalid URLs,

but couldn't trigger either of these errors. For example, I couldn't trigger
the BAD_URL error with things like "http://*.**..*
\%%0()(~!!@$^++_-=::'\|``~//<<<>>>>", and if that's not considered a bad URL by
cURL, then I don't really know what is ;-)

  • Most bad input seems to trigger error 6, and I could get error 28 with

"http://160.160.160.160/test.gif" (i.e. a valid looking URL, but no host at
that address). For error 28 added a bit about checking the site is up.

  • For 'upload-misc-error-text', just changed to middle sentence to: 'Please

verify that the URL is valid and accessible and try again.'

Attached:

nickpj wrote:

Incomplete patch (separate to the above) as described below.

One small other thing - this probably needs a separate bug, but for example on
the Incorrect Protocol page after a failed upload it says "Return to [[Main
Page]]." - ideally it would say "return to [[Special:Upload]]" (since that's
where we came from). There's a patch that does this attached, but it probably
stuffs up loads of other pages that currently use $wgOut->errorPage(). Maybe an
extra parameter with a default to false could be added somewhere to add this to
error pages, whilst retaining backwards compatibility.

Attached:

nickpj wrote:

Fixed in r17478. Comment #3 can be dealt with in a separate bug at a later date.