r62840 MediaWiki - Code Review archive

svn:keywords=Id ?

Some code style issues here.

+<?php
+if ( !defined( 'MEDIAWIKI' ) ) {
+	// Eclipse helper - will be ignored in production
+	require_once ( 'ApiBase.php' );
+}

Too much spacing, it should be just require_once( 'ApiBase.php' );

+class AssertEditApi extends ApiBase
+{
+	public function __construct( $main, $action ) {
+		parent :: __construct( $main, $action );
+	}

Braces are placed on the same line as the start of the function, or in this case, class. And a bit too much spacing in the call to parent constructor — parent::__construct( $main, $action ); is the current standard.

I copypasted that from somewhere...

Most of the API seems to fall fowl to these.

More reason stylize should do these things, maybe even on like a commit hook...

Class should be named ApiAssertEdit rather than AssertEditApi for consistency with other API classes.

AssertEdit hooks into ApiEditBeforeSave and thus has the effect of adding parameters, 'assert' and 'nassert', to action=edit. Unfortunately I can't see any way to have the extension incorporate this into the generated API help page alongside the other action=edit parameters.

I'm assuming doing what this module does, i.e. adding an 'action=assertedit' that doesn't do anything other than die with an error, is a workaround for this. As it stands, though, I can see people trying to call action=assertedit and then complaining it doesn't do anything.

At the least the description should make it clear that clients aren't meant to use the assertedit action itself, instead passing one of the specified values as assert or nassert parameter to action=edit. Better would be to find a way of having the parameter documented under action=edit where it should be. Besides which is this really needed? There is already a machine-friendly way to check if the extension is installed (meta=siteinfo + siprop=extensions) and documentation on the extension page, which anyone intending to use the assertions in a client ought to be reading anyway. (Said extension page was somewhat out of date and didn't mention the API at all -- I've updated it).

API help screens are meant to document API modules, not other random code. I think you should use a README file instead.

You can use Manual:Hooks/APIGetParamDescription and Manual:Hooks/APIGetAllowedParams to add Informationen to the API help page.

(Untested)

$wgHooks['APIGetAllowedParams'][] = 'onAPIGetAllowedParams';
$wgHooks['APIGetParamDescription'][] = 'onAPIGetParamDescription';

function onAPIGetAllowedParams( &$module, &$params ) {
	if ( !$module instanceof ApiEditPage ) {
		return true;
	}
	
	$options = array(
		'user',
		'bot',
		'true',
		'false',
		'exists',
		'test'
	);
	$params['assert'][ApiBase::PARAM_TYPE] = $options;
	$params['nassert'][ApiBase::PARAM_TYPE] = $options;
	
	return true;
}

function onAPIGetParamDescription( &$module, &$desc ) {
	if ( !$module instanceof ApiEditPage ) {
		return true;
	}
	
	$options = array(
		' user   - Verify that bot is logged in, to prevent anonymous edits.',
		' bot    - Verify that bot is logged in and has a bot flag.',
		' true   - Always true; nassert=true will fail if the extension is installed.',
		' false  - Always false; assert=false will fail if the extension is installed.',
		' exists - Verify that page exists. Could be useful from other extensions, i.e. adding nassert=exists to the inputbox extension.',
		' test   - Verify that this wiki allows random testing. Defaults to false, but can be overridden in LocalSettings.php.'
	);
	$desc['assert'] = array_merge( array( 'Allows bots to make assertions.' ), $options );
	$desc['nassert'] = array_merge( array( 'Allows bots to make negative assertions.' ), $options );

	return true;
}

Option exists looks like is a dupe to nocreate.

Thanks for that. Not sure why I didn't pay attention before. Refactored a little bit, and applied in r83181 :)