On Thu, Dec 7, 2017 at 11:07 AM Daniel Kinzler <daniel.kinz...@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 _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
