https://bugzilla.wikimedia.org/show_bug.cgi?id=17784
Roan Kattouw <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |[email protected] --- Comment #1 from Roan Kattouw <[email protected]> 2009-03-04 15:15:44 UTC --- >+ private $prefix, $module, $description; All three of these are superfluous: $description isn't even used, $prefix is only used once in the constructor and can be eliminated easily, and every usage of $this->module should be replaced by $this->getModuleName(). >+ public function execute() { >+ $this->run(); >+ } >+ >+ /** >+ * Entry point when coming from a generator >+ */ >+ public function executeGenerator($resultPageSet) { >+ $this->resultPageSet = $resultPageSet; >+ $this->run(); >+ } >... >+ private function run() { This strikes me as odd, since you could've just renamed run() to execute() here. The way the core API modules do this is with function run($resultPageSet = null), with execute() and executeGenerator() wrapping around them. (Letting execute() take an optional $resultPageSet parameter isn't allowed, since it's reimplemented from ApiBase which declares it with an empty parameter list.) >+ private function returnCount($res) { >+ $data = array("count" => $res); >+ >+ $result = $this->getResult(); >+ $result->setIndexedTagName($data, 'count'); >+ $result->addValue('query', $this->getModuleName(), $data); > >+ } This looks horribly broken; if I understand correctly, this'll try to put an object in the result, which leads to nasty errors and is not what you want. You probably want something like intval($res->getCount()) (dunno how SMW's result objects work exactly), in which case you also don't need the setIndexedTagName() call because $data doesn't have numerical indices. >+ $data[] = array( >+ 'pageid' => >intval($object->getArticleID()), >+ 'ns' => >intval($title->getNamespace()), >+ 'title' => >$title->getPrefixedText()); >... >+ $result->addValue('query', $this->getModuleName(), >$data); Since a recent breaking change in the API (r46845), your code should be prepared to handle the case in which $data doesn't fit into the result object, in which case addValue() will return false. This means you have to add the results one by one and check addValue()'s return value, like this: $vals = array( 'pageid' => intval($object->getArticleID()), 'ns' => intval($title->getNamespace()), 'title' => $title->getPrefixedText()); $fit = $result->addValue(array('query', $this->getModuleName()), null, $vals); if(!$fit) { set a query-continue and break out of the loop } ... // And after the loop: $result->setIndexedTagName_internal(array('query', $this->getModuleName()), 'page'); Note that this code only works with MediaWiki 1.15alpha; 1.14 and earlier versions don't have the setIndexedTagName_internal() method. >+ $this->resultPageSet->populateFromPageIDs($wikiPageIDs); Since you're not filling $wikiPageIDs anywhere, generator mode is also completely broken. >+ private function parseQuery($query) { >+ global $wgParser; >+ >+ // Initialize the parser options >+ $options = new ParserOptions(); >+ >+ // Preprocess the query, before running >+ $parsedQuery = $wgParser->transformMsg($query, $options); >+ >+ // Return the parsed query >+ return $parsedQuery; >+ } This could be shortened drastically to just return $wgParser->transformMsg($query, $options); . You should use $wgParser->preprocess() instead, though, since transformMsg() uses $wgTitle (which is null, which'll probably cause nasty errors). >+ global $wgRequest, $wgOut, $smwgQEnabled, $smwgQMaxLimit, >$wgUser, $smwgQSortingSupport; All of these globals are unused. >+ if ($params['_use_parser'] == true) { Just using if($params['_use_parser']) will do; the API will ensure that $params['_use_parser'] is a boolean. >+ protected function getLinker($firstcol = false) { >+ if ( ($firstcol && $this->mLinkFirst) || (!$firstcol && >$this->mLinkOthers) ) { >+ return $this->mLinker; >+ } else { >+ return NULL; >+ } >+ } This function isn't called from anywhere and uses unset member variables. I guess it can be removed safely. >+ public function getAllowedParams() { >+ return array( >+ "_query" => array( I don't get why underscores are used here. If you really want every parameter to look like &ask_foo, you should set your prefix to "ask_" instead of "ask"; parameter names with underscores in them aren't used in the core API, though. >+ "_sort" => array( >+ ApiBase :: PARAM_TYPE => "string", >+ ), You can use ApiBase::PARAM_ISMULTI => true here to allow for multiple values separated with pipes (|), which is how multivalue fields are usually done in the API. The API parameter handling code will ensure that $params['_sort'] is an array rather than a pipe- (or comma-)separated list in that case. Also note that 'string' is the default parameter type, you don't have to set it explicitly. >+ "_sort_order" => array( >+ ApiBase :: PARAM_TYPE => "string" >+ ), If the only values allowed are ASC and DESC, you'd probably be better off with: '_sort_order' => array( ApiBase :: PARAM_TYPE => array( 'asc', 'desc' ), ApiBase :: PARAM_ISMULTI => true, ApiBase :: PARAM_ALLOW_DUPLICATES => true ), Which'll do the same thing as I mentioned above, except that only the values 'asc' and 'desc' are accepted and that duplicate values are allowed. >+ "_limit" => array( >+ ApiBase :: PARAM_TYPE => "integer", >+ ApiBase :: PARAM_DFLT => "20" >+ ), There is a special limit type that lets you define different maxima for users and sysops/bots and supports limit=max (this one doesn't even *have* a maximum, which is evil). That'd look like this: 'limit' => array ( ApiBase :: PARAM_TYPE => 'limit', ApiBase :: PARAM_DFLT => 20, ApiBase :: PARAM_MIN => 1, ApiBase :: PARAM_MAX => ApiBase :: LIMIT_BIG1, ApiBase :: PARAM_MAX2 => ApiBase :: LIMIT_BIG2 ), Note that this uses integers instead of strings for DLFT and MIN (because that makes sense), and uses the constants ApiBase::LIMIT_BIG1 and BIG2 which have the values 500 and 5000 respectively. If SMW needs lower maxima, use those, of course. >+ "_offset" => array( >+ ApiBase :: PARAM_TYPE => "integer", >+ ApiBase :: PARAM_DFLT => "0" >+ ), This can be done with '_offset' => 0, as well which'll declare _offset to be an integer defaulting to 0. You'll need the array syntax if you want more advanced stuff like min and max, of course. >+ "_use_parser" => array( >+ ApiBase::PARAM_TYPE => "boolean", >+ ApiBase::PARAM_DFLT => false >+ ), Similarly, this can be done (and is usually done) with '_use_parser_' => false, >+ "_count" => array( >+ ApiBase :: PARAM_TYPE => "boolean" >+ ), Same here. I don't see why you didn't give this one a default value while _use_parser does have one, but either way the default for a boolean parameter can't be anything else than false. Also note that all of this stuff (defaults, min/max, pipe separation) doesn't need to be mentioned in the parameter descriptions: the API automatically adds this information. -- Configure bugmail: https://bugzilla.wikimedia.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
