Page MenuHomePhabricator

Drop OOUI's custom Exception class
Closed, ResolvedPublic

Description

OOUI has a custom Exception class that is thrown in a handful of places. All those places use this exception type as an unchecked exception, meaning callers must not catch it, but instead avoid it in the first place (for example, in HtmlSnippet, by making sure that the argument is of the required type). However, unchecked exceptions should inherit from SPL exceptions (LogicException & RuntimeException), as documented in https://w.wiki/6nur. Amongst other things, this allows static analysis tools (like PHPStorm and phan) to categorize the exception and set expectations such as «Should the exception be documented using @throws?» or «Should callers be catching this exception?».

However, reparenting the OOUI\Exception class is not convenient: different SPL exceptions would make sense in different places. For example, sometimes it's InvalidArgumentException, other times it could be UnexpectedValueException, etc. So, it would be easier to just ditch the custom OOUI\Exception class, and throw SPL exceptions directly. This prevents PHPStorm from complaining about missing @throws, as well as randomly adding them when refactoring code. And it will help avoiding spurious phan issues when T338426 is resolved.

There's an argument to be made of whether this can be considered a deprecation change. Unchecked exceptions (unlike their checked siblings) are implementation details and not part of a method's contract: callers should make sure to avoid the exception in the first place, so it should not matter to them what exception class is used. On the other hand, we didn't have a formal exception handling policy until T321683 (last year), so not everyone may be familiar with it. Nonetheless, it's reassuring that all usages known to codesearch are IDE-added @throws tags that are wrong and should be removed.

Finally, a special mention goes to the one place where OOUI\Exception is caught: in OOUI's Tag::__toString. Here I need to add a bit of history. Before PHP 7.4, it was not possible to throw exceptions from within __toString methods. If you did that, PHP would just fatal and catch fire: https://3v4l.org/s5Fkk. To work around this limitation, r174834 (in 2014) moved the existing __toString logic to a helper method, and made the real __toString emit E_USER_ERROR instead of throwing any exceptions directly. The end result is roughly the same. Nowadays we only support PHP >= 7.4 (and soon it's going to be >= 8.1), so this workaround is no longer needed. Therefore, we can just drop the custom exception-catching logic, and just let any exception bubble up from __toString.

Event Timeline

Change #1099816 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[oojs/ui@master] Re-apply "Drop custom Exception class"

https://gerrit.wikimedia.org/r/1099816

Hey @Daimona, can you add an initial patch that adds the appropriate deprecated since PHPDoc tag to the Exception class?

Hey @Daimona, can you add an initial patch that adds the appropriate deprecated since PHPDoc tag to the Exception class?

I could, but I'm not familiar with the deprecation process used in OOUI - for example, do you mention these in the changelog, and in what format, and is there a template to use for the commit message, etc. I could obviously try and infer these from past commits, but it's probably easier if done by someone who actually knows what they're doing ;)

Change #1100159 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[oojs/ui@master] [DEPRECATING CHANGE] Mark OOUI\Exception class as deprecated

https://gerrit.wikimedia.org/r/1100159

Change #1100159 abandoned by Eric Gardner:

[oojs/ui@master] [DEPRECATING CHANGE] Mark OOUI\Exception class as deprecated

https://gerrit.wikimedia.org/r/1100159

Change #1099816 merged by jenkins-bot:

[oojs/ui@master] [DEPRECATING CHANGE] Mark Exception class as deprecated

https://gerrit.wikimedia.org/r/1099816

Change #1100546 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[mediawiki/core@master] Update OOUI to v0.51.3

https://gerrit.wikimedia.org/r/1100546

Change #1100849 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[mediawiki/core@master] Update OOUI to v0.51.4

https://gerrit.wikimedia.org/r/1100849

Change #1100546 abandoned by Eric Gardner:

[mediawiki/core@master] Update OOUI to v0.51.3

Reason:

Abandoned in favor of v0.51.4

https://gerrit.wikimedia.org/r/1100546

Change #1155864 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/CheckUser@master] CIDRCalculator: Remove outdated try/catch wrapper

https://gerrit.wikimedia.org/r/1155864

Change #1155900 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] widget: Remove outdated try/catch wrapper from SpinnerWidget

https://gerrit.wikimedia.org/r/1155900

Change #1155864 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] CIDRCalculator: Remove outdated try/catch wrapper

https://gerrit.wikimedia.org/r/1155864

Change #1155900 merged by jenkins-bot:

[mediawiki/core@master] widget: Remove outdated try/catch wrapper from SpinnerWidget

https://gerrit.wikimedia.org/r/1155900

Change #1157444 had a related patch set uploaded (by Reedy; author: Krinkle):

[mediawiki/core@REL1_43] widget: Remove outdated try/catch wrapper from SpinnerWidget

https://gerrit.wikimedia.org/r/1157444

Change #1157445 had a related patch set uploaded (by Reedy; author: Krinkle):

[mediawiki/core@REL1_44] widget: Remove outdated try/catch wrapper from SpinnerWidget

https://gerrit.wikimedia.org/r/1157445

Change #1157445 merged by jenkins-bot:

[mediawiki/core@REL1_44] widget: Remove outdated try/catch wrapper from SpinnerWidget

https://gerrit.wikimedia.org/r/1157445

Change #1157444 merged by jenkins-bot:

[mediawiki/core@REL1_43] widget: Remove outdated try/catch wrapper from SpinnerWidget

https://gerrit.wikimedia.org/r/1157444