Page MenuHomePhabricator

Document appropriate usage of MWException
Open, MediumPublic


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 added a subscriber: awight.

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

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