Bryan.TongMinh wrote:

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.

Bryan.TongMinh wrote:

Upload module for the API

Untested patch.

attachment patch.txt ignored as obsolete

(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

Bryan.TongMinh wrote:

ApiUpload module

Updated, working patch.

attachment patch.txt ignored as obsolete

Bryan.TongMinh wrote:

ApiUpload module

Woops, uploaded the wrong patch. Updated, working patch.

Attached:

(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

Bryan.TongMinh wrote:

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.

mdale wrote:

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