Last modified: 2011-01-25 01:24:17 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.
Reassigning to self. I am currently working on a quite extensive refactoring of the upload code. This one is probably next.
(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.
Created attachment 5224 [details] Upload module for the API Untested patch.
(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
Created attachment 5227 [details] ApiUpload module Updated, working patch.
Created attachment 5228 [details] ApiUpload module Woops, uploaded the wrong patch. Updated, working patch.
(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
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.
(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.
(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.
update on this branch (as it affects the normal upload form as well) posted here: https://bugzilla.wikimedia.org/show_bug.cgi?id=18563
Module has been added with the new-upload branch merge