Page MenuHomePhabricator

Refactor ILocalizedException to be DI-friendly.
Open, Needs TriagePublic

Description

Currently all instances of ILocalizedException interface have to return a Message object from getMessageObject(), which makes them impossible to construct without using global state, cause Message object can not be constructed without global state.

The following needs to be done:

  • ILocalizedException::getMessageValue(): MessageValue should be introduced.
  • ILocalizedException::getMessageObject() should be deprecated and removed.

This requires a backwards-incompatible change, since we can't introduce new methods to an interface without breaking compatibility, but luckily only 3 extensions implement the interface directly. Introducing a whole new hierarchy of exceptions instead will not be worth it in my opinion.

Event Timeline

Sequence to do this in a backwards-compatible way, according to the stable interface policy:

  1. write email to wikitech-l
  2. introduce abstract base class with old and new method. New method is implemented based on the old, old method is abstract.
  3. make all exceptions that are implementing ILocalizedException extend the base class
  4. wait for response on email
  5. add new method to to the interface, soft-deprecate the old method
  6. change callers of old method to new method
  7. change exceptions to implement the new method as well as the old method
  8. on the base class, implement the old method based on the new, deprecate calls to the old method
  9. remove old method from all relevant exceptions

Change 720105 had a related patch set uploaded (by TChin; author: TChin):

[mediawiki/core@master] Introduce AbstractLocalizedException

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

Change 720105 merged by jenkins-bot:

[mediawiki/core@master] Introduce AbstractLocalizedException

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

Change 720813 had a related patch set uploaded (by TChin; author: TChin):

[mediawiki/extensions/FileImporter@master] Replace ILocalizedException with AbstractLocalizedException

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

introduce abstract base class with old and new method. New method is implemented based on the old, old method is abstract.

@daniel, please, please don't make this an abstract class but an interface or trait. I might miss something, but I really don't see what the benefit is when we force users to use this (and only this) as a base class. The AbstractLocalizedException class as it is now does not provide any benefit except what is effectively a single line of code that converts one object into another. Let me please do this myself in my exception classes.

I just tried to update the patch https://gerrit.wikimedia.org/r/720813 mentioned above, but had to give up. Please give me an interface. I don't care much if it contains both methods. But since there is already an interface for the old getMessageObject() method, it probably makes sense to have a narrow interface only for getMessageValue() as well.

Note: Please make sure to add the : MessageValue return type in the new interface. It's missing in the currently proposed code.

New proposal (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/721029): scratch the abstract class, make a LocalizedExceptionTrait which implements both methods needed, then we can keep ILocalizedException and maybe introduce a new interface just for the ::getMessageValue or just change ILocalizedException directly once the migration is over?

Change 720813 abandoned by TChin:

[mediawiki/extensions/FileImporter@master] [WIP] Replace ILocalizedException with AbstractLocalizedException

Reason:

Trying a different approach -- see ticket

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