Page MenuHomePhabricator

API help should always give parameters' type and default
Closed, ResolvedPublic

Description

api.php auto-generates excellent API documentation. However, it omits the type of most parameters and the default for some. It shows the type if it has a limited namespace, range, or set of values, but it doesn't simply tell you "Type: string" or "Type: integer". Similarly, it often omits the default for a parameter.

For example, look at http://en.wikipedia.org/w/api.php for action=edit. This has a redirect parameter, but there's no indication of its type or what the default behavior is. The code specifies this information:

'redirect' => array(
    ApiBase::PARAM_TYPE => 'boolean',
    ApiBase::PARAM_DFLT => false,
),

but makeHelpMsgParameters() in ApiBase.php doesn't always output $type and $default.


Version: 1.21.x
Severity: enhancement

Details

Reference
bz45652

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 1:14 AM
bzimport set Reference to bz45652.
bzimport added a subscriber: Unknown Object (MLST).

Looking at the code, it seems to me that the default will always be printed as long as it's not null or false. But many parameters use null as the default, either because there is no sane default or because the default is to skip the relevant bit entirely, which often results in the behavior of either "all" or "none"; for example, for many namespace parameters the null default is equivalent to something like 0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|100|101|108|109|710|711|446|447|828|829 (depending on the specific namespaces available on each wiki), but actually specifying that as the default would change the "where" portion of the SQL query from something like "WHERE pl_from IN (...)" to something like "WHERE pl_from IN (...) AND pl_namespace IN (0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 100, 101, 108, 109, 710, 711, 446, 447, 828, 829)".

Note that for boolean parameters the default is always false, and trying to set anything other than null or false as the default results in an error for all queries. From the client, specifying the parameter at all, even as "&redirect=no", "&redirect=false", "&redirect=0", "&redirect=", or (in GET requests) "&redirect", is interpreted as true in line with the behavior of checkboxes in HTML forms.

The indication of type should be relatively easy for someone to pick up; it should probably go somewhere in the makeHelpMsgParameters() method in includes/api/ApiBase.php.

Cleanup of any modules that use null for the default and take behavior equivalent to something that could be specified as a default should also be relatively easy, if tedious.

thobhanikishan wrote:

Wording,

1.) Fix makeHelpMsgParameters() to output default & type in all cases.
2.) Go through all the modules in the API and make sure there is a relevant default and shouldn't be left null.

Is this whats expected? Also can you please assign me to this, i would like to work on it.

(In reply to kishanio from comment #2)

2.) Go through all the modules in the API and make sure there is a relevant
default and shouldn't be left null.

While also making sure that this change doesn't result in stupid SQL queries like "WHERE pl_from IN (...) AND pl_namespace IN (0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 100, 101, 108, 109, 710, 711, 446, 447, 828, 829)".

Reading over this again, I'd lean towards just skipping item #2 entirely rather than inserting checks all over the place for "was this parameter explicitly specified?".

thobhanikishan wrote:

Reading over this again, I'd lean towards just skipping item #2 entirely
rather than inserting checks all over the place for "was this parameter
explicitly specified?".

Alright i won't touch it. So should i run a check in makeHelpMsgParameters that in case if default is not specified it will be mentioned as <NONE> / <NOT SPECIFIED>. Do you feel a place holder would make more sense?

Change 132699 had a related patch set uploaded by Kishanio:
API help should always give parameters' type and default

https://gerrit.wikimedia.org/r/132699

Looking at the result of kishanio's patch, I think this needs another iteration on the design.

I find the output needlessly cluttered by "Type: string" all over the place. This is generally useless since *everything* is a string. Sometimes the string has further restrictions like "valid title" or "user name" or "integer" which is useful to note, but just "Type: string" is more useless than asking someone what they want for lunch and they reply "Food".

I also find it useless to see "Type: boolean" and "Default: false", since there is no other default possible for a boolean type.

thobhanikishan wrote:

(In reply to Brad Jorsch from comment #6)

I find the output needlessly cluttered by "Type: string" all over the place.
This is generally useless since *everything* is a string. Sometimes the
string has further restrictions like "valid title" or "user name" or
"integer" which is useful to note, but just "Type: string" is more useless
than asking someone what they want for lunch and they reply "Food".

Seems wise. Too much redundancy. What do you suggest for exact change to-be or is there anyone who is familiar with all the modules and can guide/help me compile the set of information to be exposed here? To say a-bit more insight.

I also find it useless to see "Type: boolean" and "Default: false", since
there is no other default possible for a boolean type.

Thats what i thought but since i'm new here i still need to get familiar with protocol of what needs to be fixed/raising questions over discussion etc.

Change 199982 had a related patch set uploaded (by Anomie):
API: Document parameter types

https://gerrit.wikimedia.org/r/199982

Change 199982 merged by jenkins-bot:
API: Document parameter types

https://gerrit.wikimedia.org/r/199982

Legoktm added a subscriber: Legoktm.