Page MenuHomePhabricator

Phase out use of MWException
Open, MediumPublic

Description

As explained in T321683#8347775, MWException no longer serves the original purpose of rendering a skinned and localised error page. The logic that once relied on MWException can now handle all types of exceptions, meaning the original (de facto) reason for using MWException no longer stands true. Over the years, the MWException class has become a general-purpose exception class much like the base Exception class, with the difference that the "MW" prefix leads the developers into thinking that using this class in place of the Exception class is absolutely fine. Another consequence of the class being general-purpose is that it's sometimes unclear whether the exception should be treated as checked or unchecked, i.e., whether it should be caught. Most places treat it as unchecked, like a RuntimeException. However, there's code out there where the exception is treated as checked, which is very unsafe: anything catching MWException incurs in the risk of catching completely unrelated exceptions that aren't even meant to be caught. All in all, the only purpose of MWException nowadays is to confuse developers by offering a middle ground between checked and unchecked exceptions. For these reasons, as decided in T321683 and documented in the coding conventions, the exception has been deprecated and shall be removed.

Old description

ASchulz and OLivneh have been doing some helpful work refactoring most references to MWException as plain old Exceptions. Please document what this is about and lay out some suggestions for how to write new exception throwing and handling code going forward.

Event Timeline

awight raised the priority of this task from to Medium.
awight updated the task description. (Show Details)
awight set Security to None.
awight added subscribers: ori, aaron, Majr, Anomie.
awight subscribed.

In the hope of priming a discussion, please humor my attempt to spell out some of the ideas floating around... I won't assign "sides" to these points, because I think we share the same goals and can find alignment. Sorry ahead of time if I've paraphrased you poorly.

  • The next time there are sweeping changes to be made, however minor, please write up a paragraph about the change and link to it in commit messages. Sometimes it's worth the overhead of starting an inclusive conversation before writing patches, at other times it's fine to just plow ahead, but in both cases, communication will improve the reception. Providing a nucleus for discussion will cut down on repeated effort to explain the motivation for change.
  • MediaWiki code has too many implicit interdependencies. MWException is an example of a ubiquitous dependency which might be giving us very little benefit.
  • Even loosely coupled, highly componentized libraries might still have core dependencies. In our case, this would probably include error handling, configuration, dependency injection and logging.
  • When throwing and catching exceptions, it's often useful to assign interfaces or attributes to exceptions to allow for differential handling. If extensions are simply wrapping errors in a blanket MWException, they are hardly making use of this facility. We would be better off either subclassing errors into meaningful categories, or stripping away the unused layer and throwing bare Exceptions.
  • MWException provides many general functions which depend on other MediaWiki classes and globals. These seem to lay well outside of the class's single responsibility of encapsulating exception information, and should be moved to a friend class if that will improve the portability of MWException.

We would be better off either subclassing errors into meaningful categories, or stripping away the unused layer and throwing bare Exceptions.

Throwing a bare Exception is not a good idea. As a catch Exception then also catches PHP notices, warnings, errors that were converted to exceptions by PHPUnit and also catches other unrelated exceptions. That would make it harder than it should be to test the code and harder to use it and only catch a specific exception. Catching too many exceptions (than what one actually wants to catch) makes it harder to debug things. PHP ships with many exceptions to choose from and it is quite easy to create an empty extension of it, please do either of that.

Change 184022 had a related patch set uploaded (by Awight):
MWException -> Exception

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

Change 184022 merged by jenkins-bot:
MWException -> Exception

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

Based on T321683#8347775, should we just discourage usage of MWException, and deprecate the class? It doesn't seem to be useful, but it can certainly be confusing, especially to new developers.

Krinkle renamed this task from Document appropriate usage of MWException to Phase out use of MWException.Nov 4 2022, 7:26 PM

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

[mediawiki/core@master] Deprecate MWException

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

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

[mediawiki/core@master] Replace some usages of MWException

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

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

[mediawiki/core@master] Replace more usages of MWException

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

Change 882193 merged by jenkins-bot:

[mediawiki/core@master] Deprecate MWException

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

Change 882195 merged by jenkins-bot:

[mediawiki/core@master] Replace some usages of MWException

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

Change 882196 merged by jenkins-bot:

[mediawiki/core@master] Replace more usages of MWException

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

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

[mediawiki/core@master] ResourceLoader: Avoid new use of MWException

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

Change 907558 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Avoid new use of MWException

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

Change 928837 had a related patch set uploaded (by Physikerwelt; author: Physikerwelt):

[mediawiki/extensions/MathSearch@master] Replace deprecated MWException

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

Change 928837 merged by jenkins-bot:

[mediawiki/extensions/MathSearch@master] Replace deprecated MWException

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