[00:00:31] what do you think is the correct thing to do if an exception (of any kind) occurs after headers are sent? [00:01:19] presumably the answer should be something different, not just appending an exception message to the end of the output, with header warnings, like it does now [00:28:42] TimStarling: did you notice https://gerrit.wikimedia.org/r/c/mediawiki/core/+/696631 ? [00:29:28] yeah, it looked like a lot of code [00:29:59] I mean, a high code to commit message ratio [00:33:45] what is the bug it's fixing? [00:35:23] from the commit message I expected to see something like [00:36:12] foreach ( $indexes[0] as $index ) { if ( isset( $set[$index] ) ) throw new MWException( "Can't change the primary key in a SET clause"); } [00:36:56] not parsing and validation of SQL fragments [00:38:08] I'm just wondering whether it's worth doing -- ultimately the DBMS layer can't stop the developer from shooting themselves in the foot if they really want to, that shouldn't really be a goal [00:40:24] the question is whether changing a primary key in a SET clause is so catastrophic that it justifies 100 lines of code to prevent that from happening [01:17:27] TimStarling: I guess it could just check for the easy [ col => value ] case and be documented as not supporting PK changes [01:40:50] makes sense to me, that way we don't have a performance overhead or additional complexity [03:13:42] TimStarling, I dropped off earlier ... but, I know we ran into this in Parsoid/JS and there, we simply suppressed sending the responses after checking whether headers were sent .. since nothing useful was going to come out of either logging or sending headers again. I suppose logging isn't that bad, and ideally it wouldn't be noisy .. but, in this case Arlo is wondering why it is noisy at all. [03:14:43] short of logging, I don't see what you could reasonably do. [03:15:26] you could suppress the warning by adding !headers_sent() to the list of conditions checked by MWExceptionRenderer::useOutputPage() [03:16:02] the effect of that will be to append a basic error message to the output, potentially corrupting the API output [03:16:12] instead of appending a complete HTML page with skin [03:16:30] either way it sucks, but you can suppress that warning if you like [03:41:35] ok .. will chat with the crew and pass this on to Arlo. [22:03:37] Is it okay if we register a hook handler on the fly? Context: https://phabricator.wikimedia.org/T272032#6834213