#299 MultiUpload and ImportProjectFiles

[Lee Worden]

tracking progress on Special:MultiUpload and Special:ImportProjectFiles for MW 1.19+.

[Lee Worden]

The ajaxGetExistsWarning is working in MultiUpload - when you select a file it goes to the server and finds out if it duplicates anything on the server (*), and it puts the warning in the proper row, including a thumbnail of the file on the server...

(*) in realtime, before you submit

Related, to do:

1. Does it do the right thing for source code files?
2. With multiple uploads, we also need a check whether it duplicates something else that's in the process of being uploaded
3. What other Ajax checks are in there? Make sure they're working too. In particular, the part about licensing.

Update: it does fine with text files, though it adds a little vertical space below the destination filename field, where it would put messages about the destination if there were any.

[Lee Worden]

if IPF is going to have an Ajax check like that, it'll have to be a different one, to see if the project already has that filename, where it's stored, etc. But it won't have the "wiki already has this file" feature, because we don't have all the files hash-indexed the way the File: files are.

[Lee Worden]

Fortunately, we don't really care about that, because while WikiMedia Commons might want to avoid upload of a photograph that's already there under a different name, it's more reasonable to think people might want to upload the same file to different projects under various names. So no big deal if we don't warn about duplication away from File: locations.

[Lee Worden]

Opened a ticket on WMF Bugzilla about refactoring the MW code: https://bugzilla.wikimedia.org/show_bug.cgi?id=48581

[Lee Worden]

note to self: make sure they handle an expired edit token correctly.

Update: successful on MultiUpload.

[Lee Worden]

Thinking over what to do with the UploadBase classes. In MW Special:Upload gets the bulk of the uploading done by calling UploadBase::createFromRequest( $request ), where $request holds the contents of the submitted form fields. createFromRequest() creates an Upload object from one of several classes (UploadFromFile, UploadFromUrl) and that object has its own way of knowing what form fields to look for.

In MultiUpload I need to use different form field names: when uploading from row 2, for instance, I need to read form fields wpSourceType2, wpUploadFile2, etc, rather than just wpSourceType, wpUploadFile, etc.

I was thinking I would make a createFromRequestData(), which I would provide the field values to rather than providing the request and having the receiving object query it. But this would undermine the class polymorphism?

... in its defense, the SpecialUpload object is already reading all those fields and knows which ones are in use. But with a variable set of fields to pass to createFromRequestData, it seems like I'd have to pass an array of key-value pairs to it - which is almost equivalent to a WebRequest object...

So should I consider making a sort of stand-in request object whose 'wpSourceType' value is the form's 'wpSourceType2' value, etc?

Or I could do what Extension:MultiUpload does, and put the row's values into the main request's unnumbered fields temporarily ($request->setValue( 'wpSourceType', $this->mSourceType), etc.). But I don't like that.

I think I might try option 2, and have each row make a FauxRequest object to use in place of the real WebRequest. It looks like FauxRequest's constructor takes a nice key-value array, so this seems straightforward.

[Lee Worden]

Working on this request scheme (using DerivativeRequest, not literally FauxRequest). But I notice some of the important submitted stuff that Upload objects use is in the session, and I need to figure out how to work with that. Taking notes here.

So in the UploadFromFile class, it uses fields 'wpUploadFile' and 'wpDestFile' from the request. 'wpUploadFile' is the form's file selector, and where is the file's contents? It's retrieved using $request->getUpload( 'wpUploadFile' ). That function goes directly to new WebRequestUpload( $request, 'wpUploadFile' ), which gets the file info from $_FILES['wpUploadFile'].

This is annoying, because there doesn't seem to be a way for my fake request object to fake the contents of $_FILES - I simply have to stuff extra data into that global array. Unless I improve on the WebRequest class... :)

[Lee Worden]

So _FILES contains: (in serialized form)

a:4:{s:13:"wpUploadFile1";
a:5:{s:4:"name";s:9:"menu1.jpg";s:4:"type";s:10:"image/jpeg";s:8:"tmp_name";s:18:"/var/tmp/phpp3dPVU";s:5:"error";i:0;s:4:"size";i:74317;}
s:13:"wpUploadFile2";
a:5:{s:4:"name";s:0:"";s:4:"type";s:0:"";s:8:"tmp_name";s:0:"";s:5:"error";i:4;s:4:"size";i:0;}
s:13:"wpUploadFile3";
a:5:{s:4:"name";s:0:"";s:4:"type";s:0:"";s:8:"tmp_name";s:0:"";s:5:"error";i:4;s:4:"size";i:0;}
s:13:"wpUploadFile4";
a:5:{s:4:"name";s:0:"";s:4:"type";s:0:"";s:8:"tmp_name";s:0:"";s:5:"error";i:4;s:4:"size";i:0;}}

So it's just an array with one key for each file selector in the form, each associated with an array of information. Seems like I could add a bit to FauxRequest to support faux key-value pairs that aren't actually in _FILES.

[Lee Worden]

Hey, Special:Upload displays a thumbnail of the image being uploaded, even if there isn't a duplication warning. MultiUpload should be doing that too.

[Lee Worden]

This thumbnail is inserted by function showPreview(), in mediawiki.special.upload.js, which I hadn't looked into yet. I think this preview/thumbnail part is done all on the client, separately from the ajax bit that checks for duplicate files on the server.

It looks like I'm going to need to go through this file carefully, because there's code that assumes the thumbnail is unique on the page. I have to allow multiples and be systematic about which one to use in which operations.

[Lee Worden]

got it! in r1008. Maybe I'm getting better at this.

[Lee Worden]

remember to test uploading by URL as well (not enabled by default).

[Lee Worden]

Need to make some notes about the flow of SpecialUpload::execute() - which parts of it should be run by SpecialMultiUpload (the page) and which parts by SpecialUploadRow (each row)?

• setHeaders() : page
• outputHeader() : page
• check permissions: page
• loadRequest() : row
• cancelUpload : row
• processUpload : row
• clean up temp file : row

So I should split it in two then.

[Lee Worden]

I should have noted that the second-to-last step there is actually

• processUpload or showUploadForm, depending on what's being submitted.

Now that I've made that change to how I use execute(), showUploadForm is being handled differently. So now I'll make notes on that.

In Special:Upload, there are these two different pathways for output. If we do some processing of submitted data, we call processUpload, and if there isn't a submission to process, we call showUploadForm and await instructions. processUpload has various pathways within: it may return you to the upload form with some warnings or errors added, or succeed and does a redirect to the updated File: page, or if there's a more serious error, it will replace the entire page with an error message.

Right now, we're doing this decision tree for each row, so it needs to be adjusted - we need the form fields to be aggregated into a single form for submission, and when we succeed we need to display a success message rather than redirecting. And we need to never hijack the entire page, just put our error message within our row.

If possible, I think I want to have UploadRow::showUploadForm do nothing, and do all outputting when the page's showUploadForm calls UploadRow::descriptor() for the fields to aggregate into the one big form.

So I'll try it and see how it goes...

[Lee Worden]

All right, so that made it easy to get the initial form output looking right, at least. So now I'll go through carefully and try to get all the error, warning, and success output working right.

First, some notes on what's in there.

• SpecialUpload::showUploadForm() does showViewDeletedLinks(): this will get omitted if I don't do something about it for each row
• There's stuff in SpecialUpload::getUploadForm() that I want, using addPreText for deletion notices and the like, but I want to override the central form creation code, so I should probably split this function
• showRecoverableUploadError currently adds a message to the output form, changes the submit button, and displays it. Needs to store a message for display instead.
• showUploadWarning is great except that it does the form message and buttons and displays it. Same change.
• likewise showUploadError.
  • These could be fixed up by changing them directly, or by overriding UploadFormRow::{setSubmitText,addButton,addPreText,addPostText} and UploadRow::showUploadForm.
  • It's cleaner to change them directly, and I think it's also easier and less error-prone. I think there are three kinds of things in these functions that can be separated: managing upload data, such as calling mUpload->stashSession(); generating the message text; and outputting the message text. I'd like to do the data management outside of these calls, and split the remainder into getMessage and showMessage functions, so that I can just override the show part.
• uploadSucceeded does a redirect - can be overridden without any modification needed

This seems workable. I think I need to get clearer about how the session data is used, so I know how to handle that part right.

Update: all these things are dealt with now, except that I didn't take stashSession() out of the error-reporting functions.

[Lee Worden]

pretty sure session data and edit token are not being handled correctly right now

[Lee Worden]

OK, well lost my notes about the session key. At least they'll be better organized the second time through.

Use of session key in SpecialUpload.php:

• showRecoverableUploadError() and showUploadWarning() call $this->mUpload->stashSession(), which returns a session key, and give the session key to getUploadForm().
  • But showUploadError() doesn't stash the file - why not?
• getUploadForm() takes a $sessionKey argument
  • "String: session key in case this is a stashed upload"
  • passes it to the UploadForm constructor
• if edit token fails during a submit, getUploadForm() outputs a 'session_fail_preview' message.
• UploadForm class has a $mSessionKey member variable
  • initializes it from options passed to constructor
  • if mSessionKey is set, getSourceSection() outputs two hidden form fields instead of the what-file-to-upload interface: 'SessionKey' => mSessionKey, 'SourceType' => 'Stash'. These fields get renamed 'wpSessionKey' and 'wpSourceType' in the request that this form sends in.
  • if mSessionKey is set, getDescriptionSection() puts a thumbnail of the stashed image in the form's header.
• SpecialUpload::unsaveUploadedFile() gets rid of an existing stashed file represented by $this->mUpload.

And in UploadBase and associated classes:

• Presence of form field 'wpSourceType' == 'Stash' in the request data tells UploadBase to create an UploadFromStash object instead of an UploadFromFile.
  • It calls UploadFromStash::isEnabled(), UploadFromStash::isValidRequest(), new UploadFromStash, and then the new object's initializeFromRequest() method.
• UploadFromStash::isEnabled() just says yes.
• UploadFromStash::isValidRequest() just checks that the session key is the right kind of string. It needs to be basically a filename, something like "a12736bc.jpg".
• The constructor does generic setup.
• And initializeFromRequest() uses the 'wpSessionKey' and 'wpDestFile' fields to locate a stashed file to try to finish uploading.

Weirdly, I think all this data doesn't really have anything to do with the PHP session?! Some of the stashed file's data is stored in a database table, indexed by the "session key", and the file itself is stored in a temp directory, and maybe that's it... I could delve further into this, but I think what I should do is make sure the row objects keep good track of their sessionKey if they have one, and see if all the rest of the machinery does the right thing...

[Lee Worden]

Hehe, I lost all that stuff I just wrote too, but this time I have the recovery thing installed!

[Lee Worden]

The three (?) kinds of errors.

showRecoverableUploadError(): an error that can be handled by fixing the destination filename or description. Stash the uploaded file, show a message, let the user fix the form.

showUploadWarning(): You might want to fix something, but we also provide a "continue anyway" button because it will upload successfully as is. Stash the uploaded file, show a message, repeat the form with extra button.

showUploadError(): Display an error message, repeat the upload form, do not stash the file. Called if:

• the uploaded file is missing
• the file is received but can't be uploaded into the wiki for some reason
• the file is empty or too large
• the filetype is unacceptable
• the file contents fail a security verification
• the upload is aborted by an extension using a hook

Other places stuff is output that I should look out for:

• showViewDeletedLinks() writes directly to page. Split most of it into a separate getViewDeletedLinks().
• unsaveUploadedFile() writes an error page in case of an error condition.
• uploadSucceeded() does a redirect, as already noted.

Update: see below for progress on those three error functions. showViewDeletedLinks() and the other two at the end are taken care of as well.

[Lee Worden]

showUploadError is working now. Unfortunately I'm getting spurious "File is empty" errors in the rows that I don't fill in, because Special:Upload doesn't like empty submissions. That shouldn't be flagged as an error in MultiUpload.

[Lee Worden]

Leaving myself a to-do note on that, for when I pick up again. I'm adding a SpecialUpload::shouldProcessUpload(). UploadRow needs to figure out how to check the File object (whatever it's called) that UploadFromFile gives it, to see whether it's empty, to avoid trying to process it.

Update: dealt with, by adding a version of processVerificationError() that passes over that one when wpDestFile isn't set.

[Lee Worden]

For the record: I tested showUploadError() by hand-inserting the text of a script tag '<script src="http://bad.actor"/>' into the middle of a .jpg file. MW catches and rejects that because it could be executed by certain really messed-up implementations of Internet Explorer.

One way to test showUploadWarning() is to upload a file that's a renamed copy of something that's already been uploaded. I thought this would be caught by the ajax step before submitting, but it looks like it's handled afterward as one of these.

showUploadWarning() is working in the sense of showing an upload warning. It adds an extra check box (actually adds two and makes one of the usual ones disappear) where SpecialUpload adds two submit buttons. It appears to stash the file correctly. But I still need to test that checking the buttons and resubmitting gets the right behavior.

[Lee Worden]

Yes, and testing the buttons:

Special:Upload follows one of these warnings with three buttons:

• "Submit modified file description", which is the standard submit button with its name changed,
• "Ignore warning and save file anyway",
• "Cancel and return to the upload form".

In MultiUpload we have the standard submit button at below all the rows, and two check boxes within the row: "Ignore warning and save file anyway", and "Cancel and return to the upload form". The analogues of the three submit buttons are:

• Modify the file description and press submit,
• check the "Ignore" box and press submit,
• check the "cancel" box and press submit.

Notice that if you press submit without doing anything, as in SpecialUpload, you should just get the same warning again. (And in the case of a duplicate file, there's no way to improve the situation by modifying the form data.)

• Resubmitting without a fix does the right thing on MultiUpload: redisplay the warning.
• Submitting with "Cancel" gives you back an empty file selector, but keeps the destination filename filled, just as in S:U.
• Submitting with "Ignore" - well, it might do the right thing, but I can't tell, because I haven't got uploading and reporting success right yet.

(Note - cancelling, using the browser's back button, and clicking "Ignore" causes Special:Upload to crash and dump a PHP backtrace, because the file's no longer found in the stash. I should report and/or fix that.)

[Lee Worden]

!#*$&#(*& I thought I had a lost-form-recovery addon installed for when I write a bunch of stuff and it gets lost before I submit, where is it when I need it??!!

[Lee Worden]

I guess I lost the addon when I had to replace my laptop... got it installed again now. http://getlazarus.com/

(I already posted about this too, but it got lost without posting too, no lie :))