Page MenuHomePhabricator

Flow: improve exception handling, reuse more core
Open, LowPublic

Description

Flow bypasses a lot of core's Exception.php, so its exceptions work differently. For example, insufficient permissions or invalid action in a Flow URL on mediawiki.org should not display an errorbox containing a Fatal exception (which isn't Fatal and I think isn't counted as an exception). For example

https://www.mediawiki.org/w/index.php?title=Talk:Sandbox&topic_postId=rp7uid68p0ilj5pj&workflow=rp7to9rygxadbohw&action=edit-post

and

https://www.mediawiki.org/w/index.php?title=Talk:Sandbox&topic_postId=rp7uid68p0ilj5pj&workflow=rp7to9rygxadbohw&action=sdfasdf

Furthermore core exceptions provide more helpful information
E.g. http://ee-flow.wmflabs.org/wiki/User_talk:Maryana?topic_postId=rpj2adfchnuuw94c&workflow=rpj2adf69649ji98&action=edit-post
prints

Internal error
Insufficient permissions to access the content.
<a stack trace (or a pink errorbox)>

whereas http://ee-flow.wmflabs.org/wiki/MediaWiki:welcomeuser?action=delete gives a helpful

Permission error
You do not have permission to delete this page, for the following reasons:
The action you have requested is limited to users in the group: Administrators.

I think most Flow exception classes could inherit from core's ErrorPageError and just pass a suitable title and 'flow-error-' . $flowErrorKey, plus any additional useful debugging information in __construct. Meanwhile PermissionException could inherit from PermissionsError andthrowers should pass it the failed permission ('flow-edit-post', etc.), and Flow.i18n.php should have messages for 'action-flow-edit-post', 'action-flow-suppress-topic', etc.

I added a bunch of comments to https://gerrit.wikimedia.org/r/#/c/116631/1/includes/Exception/ExceptionHandling.php regarding this point. If I'm misguided then that file should have a comment explaining why it's done the way it is.


Version: master
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=70497

Details

Reference
bz62176

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 2:54 AM
bzimport set Reference to bz62176.
bzimport added a subscriber: Unknown Object (MLST).

I think Flow exceptions actually log to the exception.log, e.g. a "Not ALlowed"/insufficient permissions Flow error like https://www.mediawiki.org/w/index.php?title=Talk:Sandbox&topic_postId=rp7uid68p0ilj5pj&workflow=rp7to9rygxadbohw&action=sdfasdf . It would be better to log these to a Flow-specific log together with the HTTP Referer, so we can track down the page generating the bogus URL, if any.

Change 116786 had a related patch set uploaded by Spage:
subclass core's ErrorPageError & PermissionsError

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

Change 116786 abandoned by Spage:
subclass core's ErrorPageError & PermissionsError

Reason:
It's still worthwhile to integrate with these, but this probably isn't good enough.

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

Adding a seealso for bug 70497 - I'm not sure if that's what is needed, or if this ought to become a tracking bug for the currently confusing exception messages, or something else?

The above patch was abandoned, so I'm resetting to New.