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

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T17227, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 15227 - action=upload should be added to the API
action=upload should be added to the API
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
API (Other open bugs)
1.14.x
All All
: Normal enhancement with 3 votes (vote)
: ---
Assigned To: Bryan Tong Minh
:
Depends on: 14925
Blocks: 17255 16633
  Show dependency treegraph
 
Reported: 2008-08-18 16:33 UTC by Roan Kattouw
Modified: 2011-01-25 01:24 UTC (History)
10 users (show)

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


Attachments
Upload module for the API (6.48 KB, patch)
2008-08-28 20:54 UTC, Bryan Tong Minh
Details
ApiUpload module (107.46 KB, patch)
2008-08-30 08:39 UTC, Bryan Tong Minh
Details
ApiUpload module (9.62 KB, patch)
2008-08-30 08:40 UTC, Bryan Tong Minh
Details

Description Roan Kattouw 2008-08-18 16:33:01 UTC
Easier said then done. This requires a rewrite of how upload errors are handled so we don't throw HTML-formatted error messages in the client's face. CC'ing Bryan as he's the only guy who knows both the image handling code and the API well.
Comment 1 Bryan Tong Minh 2008-08-18 17:27:18 UTC
Reassigning to self. I am currently working on a quite extensive refactoring of the upload code. This one is probably next.
Comment 2 Roan Kattouw 2008-08-18 20:14:23 UTC
(In reply to comment #1)
> Reassigning to self. I am currently working on a quite extensive refactoring of
> the upload code. This one is probably next.
> 

Thanks. I took a stab at the upload code earlier, but I backed out of it because it was too huge and unfamiliar.
Comment 3 Bryan Tong Minh 2008-08-28 20:54:52 UTC
Created attachment 5224 [details]
Upload module for the API

Untested patch.
Comment 4 Roan Kattouw 2008-08-28 21:14:52 UTC
(In reply to comment #3)
> Created an attachment (id=5224) [details]
> Upload module for the API
> 
> Untested patch.
> 

Stuff that's wrong with it:
* Messages should be added to ApiBase::$messageMap
* $wgEnableUploads is unused; it should either be checked or not be global'ed
* if( $wgUser->isAllowed( 'upload' ) ) is missing a !
* Use $this->params throughout rather than switching in the middle
* Does $params['comment'] need to be set? What happens if you leave the comment blank on a UI upload?
* $nt isn't used anywhere
* No error message is thrown if $nt isn't a Title
* Permissions checking is simply skipped if $nt isn't a Title
* $result['warnings'] and $result['details'] probably need setIndexedTagName()
* if( $status->isGood() ) is missing a !
* Make up your mind about whether to set $result['imageinfo'] or not
* Add an example that doesn't use action=move
Comment 5 Bryan Tong Minh 2008-08-30 08:39:13 UTC
Created attachment 5227 [details]
ApiUpload module

Updated, working patch.
Comment 6 Bryan Tong Minh 2008-08-30 08:40:15 UTC
Created attachment 5228 [details]
ApiUpload module

Woops, uploaded the wrong patch. Updated, working patch.
Comment 7 Roan Kattouw 2008-09-01 18:17:58 UTC
(In reply to comment #6)
> Created an attachment (id=5228) [details]
> ApiUpload module
> 
> Woops, uploaded the wrong patch. Updated, working patch.
> 

* The ApiQueryImageInfo::allProps() thing is confusing, unnecessary and non-standard (other modules also have lots of props, and they don't put them in a separate function either). Indentation is more than enough of an aid to distinguish the array of props from the rest of the getAllowedParams() array
* What's the status of the FIXME if the stashed upload section?
* Also, if you want the comment parameter to default to '', just define that default in getAllowedParams(). The amfilter parameter is a good example of how to define defaults for string parameters
* It's probably cleaner to define $request = $this->getMain()->getRequest() all the way on top of execute(), since you're using the long version near there too
* The permissions check should be moved all the way up, before validating uploads that aren't allowed anyway
* Permissions are currently checked in two places. Also, if verifyPermissions() is sane, it'll check for isAllowed() too and return some sort of value indicating why permission was denied (like Title::getUserPermissionsErrors())
* Using dieUsage() for some errors and <upload result="Failure"> is inconsistent. You should only use result="Failure" when you actually have more information to convey. Also, the message map has an 'unknownerror' message, but you should really just dieUsageMsg() the error and let dieUsageMsg() discover it's not in the message map and switch to 'unknownerror', that makes adding messages easier
Comment 8 Bryan Tong Minh 2009-01-20 16:16:54 UTC
Just as a note, I committed a test version to the new-upload branch. This does not cover Roan's notes as stated in comment #7.
Comment 9 Roan Kattouw 2009-01-20 18:21:03 UTC
(In reply to comment #8)
> Just as a note, I committed a test version to the new-upload branch. This does
> not cover Roan's notes as stated in comment #7.
> 

Yup, saw it. I'm actively fixing up your API module and parts of your upload branch right now.
Comment 10 Roan Kattouw 2009-01-23 22:52:09 UTC
(In reply to comment #7)
> * The ApiQueryImageInfo::allProps() thing is confusing, unnecessary and
> non-standard (other modules also have lots of props, and they don't put them in
> a separate function either). Indentation is more than enough of an aid to
> distinguish the array of props from the rest of the getAllowedParams() array
Whoops, I see now that it does have a use.

> * What's the status of the FIXME if the stashed upload section?
Seems to have disappeared in the branch.

> * Also, if you want the comment parameter to default to '', just define that
> default in getAllowedParams(). The amfilter parameter is a good example of how
> to define defaults for string parameters
> * It's probably cleaner to define $request = $this->getMain()->getRequest() all
> the way on top of execute(), since you're using the long version near there too
> * The permissions check should be moved all the way up, before validating
> uploads that aren't allowed anyway
Did these in the branch in r46100.

> * Permissions are currently checked in two places. Also, if verifyPermissions()
> is sane, it'll check for isAllowed() too and return some sort of value
> indicating why permission was denied (like Title::getUserPermissionsErrors())
This is related to what Tim said in bug 14925 comment #11. All these functions checking stuff should be merged into one big function that returns an error array (message key + params) that can be fed straight into dieUsageMsg() and into an OutputPage method whose name I don't remember. I recommend taking the rest of Tim's advice in that comment as well.

> * Using dieUsage() for some errors and <upload result="Failure"> is
> inconsistent. You should only use result="Failure" when you actually have more
> information to convey. Also, the message map has an 'unknownerror' message, but
> you should really just dieUsageMsg() the error and let dieUsageMsg() discover
> it's not in the message map and switch to 'unknownerror', that makes adding
> messages easier
> 
I could go and fix up all the dieUsage() calls, but that's kind of pointless if the upload code is gonna be rewritten to return error arrays anyway.
Comment 11 Michael Dale 2009-04-28 22:44:38 UTC
update on this branch (as it affects the normal upload form as well)  posted here: 
https://bugzilla.wikimedia.org/show_bug.cgi?id=18563 
Comment 12 Roan Kattouw 2009-07-25 17:28:11 UTC
Module has been added with the new-upload branch merge

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


Navigation
Links