r62282 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r62281‎ | r62282 | r62283 >
Date:01:13, 11 February 2010
Author:reedy
Status:resolved (Comments)
Tags:
Comment:
Start implementation of bug 18771 - List possible errors in action=paraminfo

Base has empty array() returning method, ApiBlock has implementation of possibleErrors from code above (possibly not complete)

Can be finished incrementally, so serves as a review point for Roan
Modified paths:
  • /trunk/phase3/includes/api/ApiBase.php (modified) (history)
  • /trunk/phase3/includes/api/ApiBlock.php (modified) (history)
  • /trunk/phase3/includes/api/ApiParamInfo.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiBlock.php
——@@ -157,6 +157,17 @@
158158 'Block a user.'
159159 );
160160 }
 161+
 162+ public function possibleErrors() {
 163+ return array (
 164+ $this->parseMsg( array( 'missingparam', 'user' ) ),
 165+ $this->parseMsg( array( 'missingparam', 'token' ) ),
 166+ $this->parseMsg( array( 'sessionfailure' ) ),
 167+ $this->parseMsg( array( 'cantblock' ) ),
 168+ $this->parseMsg( array( 'canthide' ) ),
 169+ $this->parseMsg( array( 'cantblock-email' ) ),
 170+ );
 171+ }
161172
162173 protected function getExamples() {
163174 return array (
Index: trunk/phase3/includes/api/ApiBase.php
——@@ -964,6 +964,12 @@
965965 return false;
966966 }
967967
 968+ /**
 969+ * Returns a list of all possible errors returned by the module
 970+ */
 971+ public function possibleErrors() {
 972+ return array();
 973+ }
968974
969975 /**
970976 * Profiling: total module execution time
Index: trunk/phase3/includes/api/ApiParamInfo.php
——@@ -96,6 +96,7 @@
9797 $retval['description'] = implode( "\n", (array)$obj->getDescription() );
9898 $retval['version'] = implode( "\n", (array)$obj->getVersion() );
9999 $retval['prefix'] = $obj->getModulePrefix();
 100+
100101 if ( $obj->isReadMode() )
101102 $retval['readrights'] = '';
102103 if ( $obj->isWriteMode() )
——@@ -104,9 +105,11 @@
105106 $retval['mustbeposted'] = '';
106107 if ( $obj instanceof ApiQueryGeneratorBase )
107108 $retval['generator'] = '';
 109+
108110 $allowedParams = $obj->getFinalParams();
109111 if ( !is_array( $allowedParams ) )
110112 return $retval;
 113+
111114 $retval['parameters'] = array();
112115 $paramDesc = $obj->getFinalParamDescription();
113116 foreach ( $allowedParams as $n => $p )
——@@ -167,6 +170,11 @@
168171 $retval['parameters'][] = $a;
169172 }
170173 $result->setIndexedTagName( $retval['parameters'], 'param' );
 174+
 175+ // Errors
 176+ $retval['errors'] = $obj->possibleErrors();
 177+ $result->setIndexedTagName( $retval['errors'], 'error' );
 178+
171179 return $retval;
172180 }
173181

Follow-up revisions

RevisionCommit summaryAuthorDate
r62328More of bug 18771 - List possible errors in action=paraminfo...reedy21:34, 11 February 2010
r62333Update code documentation with Bryans help for r62282 and r62331reedy22:06, 11 February 2010
r62480* (bug 18771) List possible errors in action=paraminfo...reedy21:36, 14 February 2010

Comments

#Comment by Bryan (talk | contribs)   20:01, 11 February 2010

The comment of ApiBase::possibleErrors needs to indicate that the output is a full formatted message in array( info =>, code => ) style.

I would personally let possibleErrors() return an unformatted array and let ApiParamInfo do the formatting (separation of formatting and backend), but it probably doesn't matter that much.

#Comment by Reedy (talk | contribs)   20:13, 11 February 2010

Indeed, it doesn't matter a huge amount either way. Roan, preference? :)

And I will do on the comment. It is still work in progress.

Thanks Bryan :)

#Comment by Catrope (talk | contribs)   20:18, 11 February 2010

This separation is not as easy as it seems, because some modules use dieUsageMsg() (like this one), but others use dieUsage(), and yet others mix the two. Their signatures are incompatible, so this is already kinda sane.

I do agree that we need to get rid of these parseMsg() calls all over the place, so how about 

array(
    array( 'code' => 'foo', 'info' => 'bar' ), // for dieUsage() calls
    array( 'msgkey', 'param1', 'param2' ) // for dieUsageMsg() calls
);

And have a function in ApiBase to transform that to the format ApiParamInfo currently expects. That way other code can also obtain a consistently structured version and the parseMsg() calls will be in one place.

#Comment by Bryan (talk | contribs)   20:19, 11 February 2010

The missingparam errors can be constructed by the parent and then array_merged by the child: return array_merge( parent::possibleErrors(), array( ... ) )

#Comment by Bryan (talk | contribs)   20:20, 11 February 2010

Also function names should start with a verb, e.g. getPossibleErrors() rather than possibleErrors(). See Manual:Coding_conventions#Functions.

#Comment by Reedy (talk | contribs)   22:09, 11 February 2010

r62331

Status & tagging log