[17:50:08] What's the best way to change the signature of a method in a class that's stable to extend? Most of the remaining usages of $wgOut in core (T252979) and other extensions are in classes extending MWException, specifically in the report() method, and I would like to add an $output parameter to that, but that would be a breaking change, and I'm not sure how to get around that (e.g. adding another method to replace report() also seems [17:50:09] T252979: MediaWiki core needs uses of $wgOut removed - https://phabricator.wikimedia.org/T252979 [17:50:09] complicated) [17:50:57] ErrorPageError, which extends MWException and overrides report(), adds an $action parameter to it, which doesn't make it any better [17:51:58] MWException is deprecated but replacing it with something else seems like a lot more work than somehow changing the signature [18:54:15] we have been slowly getting rid of MWException. there are actually not that many uses left: https://phabricator.wikimedia.org/T328220 [18:54:37] (and ErrorPageError probably shouldn't extend MWException, but i don't know how hard that would be to do) [18:56:46] i don't think it's possible to add a parameter to a method that's stable to extend without violating the policy. we run into this often with hooks. you have to add a new method (and you can deprecate the old one) [19:03:52] hm, with a new method I don't think it's possible to emit deprecation warnings then if the old method is still overridden, can it be hard deprecated and removed without warnings in that case? [19:04:36] and I assume as long as the old method still exists, the only thing the new one can do by default in ErrorPageError is calling the old one for now since otherwise it would break existing classes [19:05:36] since there are only a few callers according to codesearch, it might also be possible to create a new method like setOutput() instead and call that before calling report(), then deprecate calling report() if setOutput() was not called before [19:05:57] that would only require one deprecation but is not as clean as passing an OutputPage to report i guess [20:15:31] ideally MWException::report() should be killed in favor of some kind of callback or handler registry mechanism in MWExceptionHandler::report() [22:28:43] afaik we already don't rely/support customizing that. It's mainly catch statements that depend on it in various places. [22:28:56] the class itself that is, as a type, not its contents per se [22:29:05] we already copied/moved out most of it over the years [22:29:13] so that plain exceptions get the same level of pretty rendering [22:30:08] https://codesearch.wmcloud.org/deployed/?q=extends+MWException%7Cfunction+report%5Cb&files=php&excludeFiles=test&repos= [22:30:39] https://codesearch.wmcloud.org/deployed/?q=extends+MWException%7Cfunction+report%5Cb&files=%28Exception%7CError%29.*php&excludeFiles=test&repos= [22:31:07] one weird thing in Wikibase, and Flow [22:32:22] maybe we need a getter for http status code [22:33:12] or maybe flow should use a regular exception subclass, catch it, and render its error page a bit lower down rather than relying on global handlers [22:34:19] https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1243233 is what my idea was for ErrorPageError, it wouldn't rely on wgOut anymore this way after deprecation (at least from ActionEntryPoint) except if ->report is called directly [22:34:36] not sure if that's a good approach though since I'm not really familiar with this part of core [22:35:24] I agree that it makes sense to separate the handling in flow [22:43:41] I haven't looked into *all* subclasses but I think ErrorPageError wouldn't necessarily need to depend on MWException anymore after this change [23:07:50] SomeRandomDev: Hm.. is that `catch ( ErrorPageError )` the only caller to report()? [23:08:07] I assumed the global error handler did so based on MWException, rather than based on ErrorPageError [23:08:30] also, we'd need to check all code that catches MWException to determine if it can/needs to catch ErrorPageError [23:08:41] The second caller is the global error handler I believe [23:09:06] https://gerrit.wikimedia.org/g/mediawiki/core/+/cd5dc81a547ba9de6b7763ab136603e09f8dea85/includes/Exception/MWExceptionHandler.php#121 [23:09:19] Seems like hasOverriddenHandler() is doing this kind of detection already [23:09:23] Though shouldn't ideally an ErrorPageError not end up there [23:09:45] or is that assumption wrong [23:10:21] ideally, there are no uncaught exceptions [23:10:36] handling exceptions measn we have to assume that somehow it did [23:10:46] although granted, ErrorPageError are often used for seeminglyh not-very-exceptional scenarios [23:11:02] so maybe that'd be an exceptional exception, a mistake in its own right [23:11:22] might be worth placing a logger or stat increment to find out over a month or two [23:11:40] we could also extract MWException::report into an interface that's implemented by MWException and ErrorPageError [23:11:45] that leaves intermediary catch statements across the code bases [23:12:00] MWException is de-facto the interface for "I make my own report" [23:12:06] it has no further purpose anymore [23:12:09] hm [23:12:28] ErrorPageError is a subset of those use cases [23:12:56] if you throw a custom MWExceptionj with report(), the ActionEntryPoint wont call report because it's not an ErrorPageError [23:13:13] the global one will instead [23:13:46] so if I understand it correctly we should verify whether that subset of ErrorPageErrors will actually end up in the global MWExceptionHandler, and if not it doesn't need to extend MWException? [23:14:04] since the two would be separate then and could therefore be separately handled [23:14:07] https://codesearch.wmcloud.org/deployed/?q=extends+MWException%7Ccatch.*MWException&files=&excludeFiles=test&repos= [23:14:44] I'd be nice if we could have 0 MWException catches, besides MWExceptionHandler [23:14:52] that's also a good idea [23:14:54] there's not many left [23:15:04] e.g. WebRequest::getGlobalRequestURL should use a LogicException instead [23:16:28] I don't see anything about this in the stable interface policy but what's the process for changing the type of exception a method throws? [23:16:36] Wikibase and Flow both subclass ErrorPageError, not MWException [23:16:48] so it appears that, outside MW core, there are no MWExceptino subclasses that implement report() today [23:16:54] oh [23:17:12] do check my work, but I think that's true? https://codesearch.wmcloud.org/deployed/?q=ion+report%5Cb&files=%28Exception%7CError%29.*php&excludeFiles=test&repos=#mediawiki/extensions/Flow [23:17:41] yeah I think so too [23:18:36] so we could add an exception for ErrorPageErrors instead of MWException in the global error handler? [23:18:43] since MWException::report falls back to it anyway [23:18:52] yeah, we might be there [23:18:58] that's nice [23:18:59] I thikn thats what the deprecation was working towards [23:19:25] if you trace when that was added, you might find a patch or task that planned to do exactly that [23:19:43] there would be a deprecation warning if they implemented report [23:20:05] would there? [23:20:13] the detectDeprecatedOverride call in MWException doesn't specify a version [23:20:23] ah, it's reusing that code for its logic, not for making a deprecation. [23:20:25] cscott: does https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1240461 look good? I rebased it on top of a parent commit that adds the back-compat parser tests. [23:20:48] messy :) [23:23:53] I have to go but I'm gonna look into this in the next days, seems like it should be possible to make a lot of progress here without having to change too much code in extensions [23:24:16] thanks for all the help/info btw [23:26:10] yw, I'll see ya in code review