Last modified: 2011-01-25 01:24:33 UTC
Upload code terribly mixes backend stuff with interface code. Interface code belongs to SpecialUpload, while backend stuff needs its own classes: class UploadFromBase {} class UploadFromUpload extends UploadFromBase {} class UploadFromUrl extends UploadFromBase {} class UploadFromStash extends UploadFromBase {} SpecialUpload should probably decide which ones to call. All classes will have an initialize( &$request ) method which is called by the constructor with $wgRequest by default but could be called with a FauxRequest by a subclass. Permission checking should be moved to a static method, either in SpecialUpload or UploadFromBase.
I will have to break backwards compatibility in some hooks. For now it is UploadComplete which passes a reference to the SpecialUpload object. I think the best way to do this is moving UploadComplete to UploadFromBase and add a new hook SpecialUploadComplete to allow content modification.
UploadFromMediawiki or UploadFromImport or something similar would also be nice.
Created attachment 5198 [details] Work in progress - 1 Current work in progress
Created attachment 5199 [details] Work in progress - 1.1 Proper patch this time. Note that it won't apply cleanly unless you svn cp includes/specials/SpecialUpload.php includes/UploadFromBase.php
Created attachment 5205 [details] Work in progress - 2 My second patch, which should be pretty much complete. Don't forget the svn cp includes/specials/SpecialUpload.php includes/UploadFromBase.php
Done in r40091.
Created attachment 5323 [details] Update Reverted by Tim Starling. I did some more work on it, but I don't have much time the coming weeks. So perhaps if somebody wants to test the code or take it over from here that would be great. Some comments from Tim on the current state: > The only reasonable application, it seems to me, is to replace > Special:Upload with something else entirely -- say an API. The design > looks OK from that perspective, but I think this would be a missed > opportunity for generalisation. There's only a few more changes that > need to be made to allow upload sources to be extended dynamically. > > I'd like to see multi-file upload in the core, and a Java applet > extension, but UploadBase isn't generic enough for that either. > UploadFromUrl actually calls $wgOut->showErrorPage(), which really kills > any kind of alternate UI. And the behaviour of > UploadBase::stashSession(), storing individual files under random keys > directly to $_SESSION, would appear very inelegant if it were applied to > a multi-file situation or alternate client situation.
Created attachment 5364 [details] Another update Another update. I'm not really happy with it and also don't have much time for it. I would love if somebody who is better at engineering plugin interfaces could look at this.
Tagging as need-review; calling on Tim to review this and see if it addresses the issues he brought up.
(In reply to comment #9) > Tagging as need-review; calling on Tim to review this and see if it addresses > the issues he brought up. > Bump. This is needed for a pretty major feature (API upload), and has been waiting to be reviewed for nearly 3 months now.
* Update the patch and Upload*.php, needs to incorporate SpecialUpload.php changes between r41253 and r44701 * UploadForm::internalProcessUpload() has too much boilerplate, which would have to be reproduced in each new UI component. I suggest merging verifyPermissions(), fetchFile(), verifyUpload(), checkWarnings(), performUpload() and cleanupTempFile() into a single function. It should accept an associative array as its parameter, with members such as initial page text, ignore warnings, comment and user. Preferably it would return a Status object. * Get rid of the UploadBase status constants, they're not extensible. Use message names for status codes. You can wrap them in Status objects where convenient. * Fix the UploadBase::BEFORE_PROCESSING case in processUpload() * Fix stupid comment "It will show an error form on failure. No it will not." * Rename UploadFromStash to UploadFromSession, document it to the point where ordinary humans might be able to work out how to use it. Rename stashSession() to createSession(), rename unsaveUploadedFile() to destroySession(). Document UploadFromSession::initialize(). Document the fact that these three functions go together. * Suggest renaming "UploadFromUpload" to something less vague, maybe "UploadFromWebForm" * "Upload handlers. Should probably just be a global" -- indeed it should, then it would be extensible. But the global should allow for arbitrary class names, instead of prepending "UploadFrom", so that extensions can maintain a naming scheme where class names start with the name of the extension. * Document all functions, including parameter definitions, with an extension writer audience in mind. It won't take long.
Adding this as a blocker to 18563. This has been done in the new-upload branch and will become a part of core when that gets merged.
New-upload was merged to trunk, this is fixed there.
