Page MenuHomePhabricator

Rework API error reporting
Closed, ResolvedPublic

Description

Error reporting from the API urgently needs work. The dieUsage() function originally designed to signal usage errors to bot owners has become the default way of reporting errors from the API, but is not suitable for showing detailed localized messages on the UI.

After some discussion, Anomie and I came up with the following plan (as far as I recall):

  • The API needs to return message keys and parameters for errors and warnings, for client side localization. The

messages are returned as <message> elements in the <error> resp <warning> section.

  • Error codes are no longer needed, message keys can be used as machine readable identifiers
  • For backwards compatibility, the English translation of the message(s) (and, for errors, and error code) should be returned to the called. This can skip messages from the database.
  • The old behavior should be retained as the default for the <error> and <warning> sections. The new behavior can be triggered using appropriate parameters (perhaps apimessages=true or some such)
  • For convenience, the API can be instructed to include a translated HTML version of the messages in the output, in the user's language or some language given as a parameter (e.g. triggered using apimessage=html & apilang=fr), considering customization using message pages. The text is returned in a <html> element in the <error> resp <warning> section.

To allow the new functionality, the ApiBase class needs to support new functions for reporting errors and warnings:

  • dieMessage( Message $msg ): report the given message as an error and abort. If old style output is requested, use the message key as the error code and use the English translation of the message as the error info.
  • warnMessage( Message $msg ): report the given message as a warning and continue. If old style output is requested, use the English translation of the message in the <warning> section.
  • handleStatus( Status $status ): use the messages from a Status object
    • if the status is fatal, report any error messages in the Status object as errors and abort.
    • if the status is not fatal but has errors or warnings, report all messages in the Status object as warnings and continue.
    • else, just carry on.

For backwards compatibility, the old reporting functions should be retained (but deprecated). They should implement a sensible default when used with the new style output:

  • dieUsage( $info, $code ): if the new style of error reporting is requested, this will generate a generic message (perhaps api-usage-error) with $info and $code as the two message parameters.
  • dieUsageMsg( $error ): if the new style of error reporting is requested, report the message given as an error. Otherwise, keep using the old hard coded message-key-to-error-code mapping in ApiMain::$messageMap.

Example of the new behavior:

<error>
  <message key="something-went-wrong">
    <param>Foo</param>
    <param>23</param>
  </message>
  <html lang="de">
    &lt;p&gt;Mit "Foo" ist etwas schiefgegangen (ID: 23).&lt;/p&gt;
  </html>
</error>

<warning>
  <message key="some-stuff"/>
  <message key="other-stuff">
    <param>X</param>
  </message>
  <html lang="de">
    &lt;ul&gt;
      &lt;li&gt;So Zeug!&lt;/li&gt;
      &lt;li&gt;Noch mehr Zeug: X!&lt;/li&gt;
    &lt;/ul&gt;
  </html>
</warning>

Example of old style output for dieError( wfMessage( "nasty-error", "X" ) ):

<error code="nasty-error" info="Nasty Error with X"/>

Example of new style output for dieUsage( "Nasty Error", "silly-code" ):

<error>
  <message key="api-usage-error">
    <param>Nasty Error</param>
    <param>silly-code</param>
  </message>
</error>

Version: 1.21.x
Severity: enhancement
URL: http://www.mediawiki.org/wiki/Requests_for_comment/API_roadmap#Errors_and_Warnings_Localization
See Also: T47277: Improve localizable error messages for API calls

Related Objects

Event Timeline

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

Looks like generally a good outline to me. A few comments:

  • While MediaWiki hopefully has enough anti-IE-stupidity code in place, avoiding the string "<html>" in the XML output format would probably be for the best.
  • Returning $msg->useDatabase( false )->text() is not just for backwards compatibility (although the back-compat output format will have to do that), it's also for bots that output to a text log or console where on-wiki-customized HTML messages don't make sense. $msg->useDatabase( true )->parse() is, of course, for UI callers where on-wiki-customized HTML messages do make sense.
  • I think <message key="some-stuff" rendered="..."/> and allowing the client to combine the rendered values would be better than concatenating them all into <html> as you have it shown. Of course, the back-compat output would continue to concatenate all the plain-text warnings under a "*" key like it does now.
  • For the back-compat output, dieMessage() should probably look up the key in the static array the same way dieUsageMsg() does now, for the code if not also for the text. But it should of course not choke like dieUsageMsg() does if the key isn't in the static array.

(In reply to comment #2)

Duplicate of bug 35074?

Possibly, though this bug is really a todo item for a concrete spec. We could mark bug 35074 as depending on this one, since implementing what is proposed here would solve that bug.

A few months ago I wrote a few notes on how to improve errors & warnings in the API roadmap RFC (make sure to expand the implementation section):
http://www.mediawiki.org/wiki/Requests_for_comment/API_roadmap#Errors_and_Warnings_Localization

I will try to keep that spec in sync with the ideas proposed here, but feel free to modify that page, so we can keep one "current proposal". Looking for good bits in the bug comment is not as good, and some good ideas might be accidently overlooked :)

An additional idea, induced by looking at bug 44111:

When catching a ErrorPageError, the Message it contains should be passed to dieMessage(), so the caller's preferences as to localization and representation of the error are taken into account.

Note that's orthogonal to the actual bug in 44111, which has to do with what is written to the server logfile rather than what is output to the client.

[replacing wikidata keyword by adding CC - see bug 56417]

For convenience, the API can be instructed to include a translated HTML version of the messages in the output, in the user's language or some language given as a parameter (e.g. triggered using apimessage=html & apilang=fr), considering customization using message pages. The text is returned in a <html> element in the <error> resp <warning> section.

Forgive my ignorance. I have not seen the apilang parameter before but a uselang parameter instead. What's the difference?
Could the API also use the Accept-Language header to select a language for the localization?

Forgive my ignorance. I have not seen the apilang parameter before but a uselang parameter instead. What's the difference?

Allowing for different languages for developer-oriented error messages versus end-user-oriented content output.

Could the API also use the Accept-Language header to select a language for the localization?

Could, but won't since it complicates things in several ways.

Ok, then in the app case we'd probably want to use the uselang parameter since we'd want to display that to the end user.

Anomie moved this task from Needs Code to In Dev on the MediaWiki-Action-API board.
Anomie set Security to None.

That's basically a duplicate of this bug, really.

Drive-by comments: defaulting to html for error messages might not be a good idea. On the other hand, the mix if html and non-html is not good either. Should we do both by stripping html into plaintext?

We should also consider making it more explicit what kind of errors the API can return so that clients can be prepared. I am also wondering how to strike the balance of forcing clients to use proper errors related to the user's task instead of generic "failed to save the page".

Would it be possible to loop-in frontend standard groups to think of some standard ways of displaying errors and warnings instead of every system having to do it from scratch?

Note I'm not following exactly the design laid out in the current description of this task, for example error codes aren't going away and the message key+params aren't returned in conjunction with text/html output of the message.

Drive-by comments: defaulting to html for error messages might not be a good idea. On the other hand, the mix if html and non-html is not good either. Should we do both by stripping html into plaintext?

The default is the backwards-compatible "plaintext" output, which I'm currently implementing as ->text() (so {{PLURAL}} works) followed by a pass to replace <var>, <kbd>, <samp>, and <code> with quotation marks, remove other HTML tags (with no attempt at semantic awareness of block tags or the like), and unescape HTML entities to give something resembling plaintext.

If the client wants non-BC output, it's responsible for explicitly asking for "plaintext" (as above), "wikitext" (->text()), "html" (->parse()), "raw" (->getKey() and ->getParams()), or "none".

If at some point in the future we decide to not have BC format be the default, we can decide then which format should be.

We should also consider making it more explicit what kind of errors the API can return so that clients can be prepared.

That's almost impossible to do since non-API hooks can easily wind up exposing new errors to the API, which is why the existing attempt along those lines was removed in rMWf0a6435f3b72: API: Remove action=paraminfo 'props' and 'errors' result properties.

I am also wondering how to strike the balance of forcing clients to use proper errors related to the user's task instead of generic "failed to save the page".

Would it be possible to loop-in frontend standard groups to think of some standard ways of displaying errors and warnings instead of every system having to do it from scratch?

This task makes it possible for the client to fetch localized text for error messages, but whether or how any client actually uses them is outside the scope of this task.

Change 321406 had a related patch set uploaded (by Anomie):
API: i18n for warnings and errors

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

Change 321406 merged by jenkins-bot:
API: i18n for warnings and errors

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

Change 331232 had a related patch set uploaded (by Matthias Mullie):
Use parsed HTML error responses instead of api-error-*

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

Change 331232 merged by jenkins-bot:
Use parsed HTML error responses instead of api-error-*

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

Change 604389 had a related patch set uploaded (by AlQaholic007; owner: AlQaholic007):
[mediawiki/core@master] WIP Puppeteer: Page should be previewable

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