Last modified: 2011-01-25 01:24:33 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 14925 - Refactor upload code to split backend and interface
Refactor upload code to split backend and interface
Product: MediaWiki
Classification: Unclassified
Uploading (Other open bugs)
All All
: Normal enhancement with 2 votes (vote)
: ---
Assigned To: Bryan Tong Minh
: patch, patch-need-review
Depends on: 18563
Blocks: 15227 15676
  Show dependency treegraph
Reported: 2008-07-25 20:36 UTC by Bryan Tong Minh
Modified: 2011-01-25 01:24 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---

Work in progress - 1 (258.49 KB, patch)
2008-08-19 14:34 UTC, Bryan Tong Minh
Work in progress - 1.1 (94.56 KB, patch)
2008-08-19 14:43 UTC, Bryan Tong Minh
Work in progress - 2 (107.46 KB, patch)
2008-08-21 19:00 UTC, Bryan Tong Minh
Update (47.21 KB, patch)
2008-09-12 22:33 UTC, Bryan Tong Minh
Another update (48.79 KB, patch)
2008-09-26 20:38 UTC, Bryan Tong Minh

Description Bryan Tong Minh 2008-07-25 20:36:13 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.
Comment 1 Bryan Tong Minh 2008-07-26 21:18:21 UTC
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.
Comment 2 Bryan Tong Minh 2008-07-29 13:14:29 UTC
UploadFromMediawiki or UploadFromImport or something similar would also be nice.
Comment 3 Bryan Tong Minh 2008-08-19 14:34:29 UTC
Created attachment 5198 [details]
Work in progress - 1

Current work in progress
Comment 4 Bryan Tong Minh 2008-08-19 14:43:13 UTC
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
Comment 5 Bryan Tong Minh 2008-08-21 19:00:45 UTC
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
Comment 6 Bryan Tong Minh 2008-08-27 18:14:37 UTC
Done in r40091.
Comment 7 Bryan Tong Minh 2008-09-12 22:33:27 UTC
Created attachment 5323 [details]

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.
Comment 8 Bryan Tong Minh 2008-09-26 20:38:00 UTC
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.
Comment 9 Roan Kattouw 2008-09-26 20:55:48 UTC
Tagging as need-review; calling on Tim to review this and see if it addresses the issues he brought up.
Comment 10 Roan Kattouw 2008-12-16 21:12:32 UTC
(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.
Comment 11 Tim Starling 2008-12-17 04:45:28 UTC
* 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.
Comment 12 Chad H. 2009-07-01 01:18:48 UTC
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.
Comment 13 Chad H. 2009-07-23 02:49:15 UTC
New-upload was merged to trunk, this is fixed there.

Note You need to log in before you can comment on or make changes to this bug.