[21:12:35] Krinkle: I'm confused by your comments on https://gerrit.wikimedia.org/r/c/999124 which mostly seem to be "other people are doing it wrong so we should keep doing so", but maybe I've misunderstood? [21:14:38] Krinkle: no, not exactly. If I understand correctly, the goal is to stop using bare Exception. In a few of these, you picked from the various available choices "InvalidArgumentsException" even for cases not relating to arguments, for when we "just" want to obtain a stack trace. However, I think RuntimeException makes for a better fit there, it's a bit more neutral, and it also happens to be what previous committers have chosen when faced [21:14:38] with the same choice of a dozen built-in subclasses. [21:15:14] Hmm. RuntimeException is meant to be things that are a run-time concern, like "database is down". [21:15:25] It's not for "I need a string and you gave me null". [21:16:00] (Also I don't really think this should be something phan is pushing, but oh well.) [21:18:44] I see Runtime as referring to something we can only know at runtime, as opposed to something where a static problem exists in how the code is written (i.e. passing a bad argument). Database integration I'd say indeed falls under that, as well as e.g. files that are missing. But the reason for the error is moot I think since these aren't errors at all, they just capture a stack trace for logging purposes. We only instantatie the Exception [21:18:44] object in order to let Monolog format it into a stack trace. [21:19:17] If one of these is about issues like a string being passed, I retract what I just said :) [21:21:04] if we want to revert previous commits or change existing code that's fine, but afaik the existing code in many diff repos were all written with the knowledge that other exception classes are availalbe, and authors/reviewers alike seem to have thought about it and chosen RuntimeException. I recall various CR threads that considered the choices and what they thoguht was suitable. [21:22:03] my CR-1 is for deviating from that without justification. I assumed it wasn't an intentional change but just a new choice you made based on seeing the list on php.net and picking something that feels right. [21:22:55] Note I only flagged the lines that instantiate this for logging. I agree most of the issues are invalid args, and those are all fine! [21:25:14] I suppose we could create a direct subclass of Exception called FakeExceptionToMakeMonologGiveUsAStackTrace? [21:27:22] or we can continue using RuntimeException :) The name does show up in Logstash fwiw. [21:27:37] * James_F sighs. [21:27:45] OK, I'll do it, under protest. It's conceptually wrong but eh. [21:52:30] James_F: you're making me think of HoC: https://www.youtube.com/watch?v=74rG63xznlw ;) [22:38:50] if you ask me, RuntimeException is correct for any unchecked exception. some other class may be correct too, but it's never worth thinking about.