Page MenuHomePhabricator

Flow: unrecognized and unsupported URL actions throw fatal errors
Closed, ResolvedPublic

Description

Bug 60834 complains that ?action=info doesn't work on a Flow page. Perhaps action=info should display useful information, see discussion at that bug.

The issue at hand is whether Flow should ignore unrecognized/unsupported actions, as it does now -- you just see the Flow board. That's misleading if someone follows a URL that used to do something useful before Flow was enabled on the page.

On a regular wiki page ?action=BAD results in

No such action
The action specified by the URL is invalid. You might have mistyped the URL, or followed an incorrect link. This might also indicate a bug in the software used by {{SITENAME}}.

(enwiki customizes the second, <nosuchactiontext>, message).

Maybe Flow should respond similarly, with a <flow-nosuchactiontext> changed to "... is invalid for a Flow board. ..."


Version: master
Severity: normal
URL: https://www.mediawiki.org/wiki/Talk:Sandbox?action=BAD
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=62173

Details

Reference
bz60936

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:00 AM
bzimport set Reference to bz60936.

I agree with not silently failing.

Some of the actions rather than failing should go to the relevant Flow-action. So for example, ?action=history should redirect the user to ?action=board-history.

+1 to Kunal – where possible, we should just drop the unsupported query string and take the user to the page (history, board, etc.). That's what mediawiki does as a fallback; that's what we should do, too.

I don't know what's changed in the code since when this bug was filed, but unsupported actions are now throwing fatal errors as can be seen on the provided url. Adjusting priority accordingly.

We're OK with the current behavior (show unrecognized action message and display exception)

No, displaying the exception is not ok. That means it's also going in the error logs, even though it's not a true exception.

Not quite :) Flow's InvalidActionException returns false for MWException::isLoggable(), so nothing is logged. So unless this happens to a Flow engineer, the fingerprinted pink

[46a903b5] 2014-04-24 05:21:54: Fatal exception of type Flow\Exception\InvalidActionException

in production isn't very useful, since the 46a903b5 fingerprint isn't written anywhere. Maybe Flow should log a backtrace to wfDebugLog( 'Flow', )

Bug 62176 "improve exception handling, reuse more core (edit)" is related.

(In reply to spage from comment #6)

So unless this happens to a Flow engineer

Flowgineer? Flowician? Flowologist?

(In reply to spage from comment #6)

Not quite :) Flow's InvalidActionException returns false for
MWException::isLoggable(), so nothing is logged. So unless this happens to a
Flow engineer, the fingerprinted pink

[46a903b5] 2014-04-24 05:21:54: Fatal exception of type

Flow\Exception\InvalidActionException

Huh, I didn't realize that. I still think that displaying the standard exception info is wrong, and will upload a patchset for it in a moment.

Change 129384 had a related patch set uploaded by Legoktm:
Catch and specially handle InvalidArgumentException

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

Change 131455 had a related patch set uploaded by Matthias Mullie:
Catch and specially handle InvalidArgumentException

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

Change 131455 merged by jenkins-bot:
Catch and specially handle InvalidArgumentException

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

Change 129384 abandoned by Legoktm:
Catch and specially handle InvalidArgumentException

Reason:
I05d6be4a59db57c24bd9a11c499cb69f18feaa41 is a better implementation.

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