On Thu, Dec 7, 2017 at 11:07 AM Daniel Kinzler <daniel.kinzler(a)wikimedia.de>
wrote:
Am 07.12.2017 um 19:48 schrieb Chad:
Basically the short version is: exceptions should
only be shown to users
in
the situation of *actual software errors*.
They're the exception, not the
norm. What we *should* do in such situation is log the error (at the
ERROR/WARNING/etc level as appropriate) and then gracefully fall back.
I agree with the idea, but I'd like to point out that log-and-fall-back
should
*not* be the normal way to handle things, IMHO.
Let's take for example an invalid title. If a user supplied an invalid
title,
they should get a helpful error. There is no reason to even log this,
really.
This should be done by code that expects to handle user input, and can
involve
throwing and catching exceptions, or be handled some other way.
We're on the same page here. It doesn't inherently need to be logged,
that's why I qualified it with "as appropriate." If it's something
where
the user just typed something bogus, we should return a helpful error to
them and move on. And the emphasis in what you say here *really* needs to
be on CATCHING the exception. Throwing with no catch is what's evil :)
Code that does not expect raw user input usually
SHOULD thrown an
InvalidArgumentException if it gets invalid input, though! Something went
wrong,
so we should fail fast & safe!
Indeed, I have no qualms with this. The point I'm making is on
user-generated actions.
We should however improve how we show exceptions to
users. We have "nicer"
handling for MWException than for other exceptions. I can think of no good
reason for this distinction - can't we do the nicer handling for all
exceptions?
Because our exception handling code is crap. Nobody's had the cycles to
work on it. Volunteers welcome ;-)
MWException is *very* heavy weight, and sadly doesn't let us use native PHP
extensions that make more sense. InvalidArgumentException is descriptive,
MWException is not. And a MWInvalidArgumentException that extends
MWException just to be descriptive seems overkill.
In an ideal world: we don't need MWException at all, or else it becomes a
lightweight wrapper exception for any other type on demand. I'm thinking
something like
throw MWException( 'InvalidArgumentException' )
Who knows?
The log-and-fall-back case should be quite rare, and
be reserved for
compatibility code - compatibility with old data, in particular. So
perhaps the
invalid title comes from the page or the pagelinks table - that's
unexpected,
but not impossible: someone may have fiddled with $wgLegalTitleChars,
rendering
once-valid titles invalid. So in that case, log-and-fall-back is the
correct
behavior.
Indeed. Not all bad behavior needs logging (or sometimes, just at a
DEBUG/INFO level because data is nice but yelling at log watchers is not).
Details all tbd, but I don't think you and I disagree here much at all :)
-Chad