Page MenuHomePhabricator
Create Task
Maniphest T287405

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

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.

Details

SubjectRepoBranchLines +/-
Stop binding against core's ILocalizedException interfacemediawiki/extensions/CentralNoticemaster+3 -12
Replace remaining ILocalizedException::getMessageObject callsmediawiki/extensions/WikibaseLexememaster+27 -62
Make use of upstream assertStatusError in testsmediawiki/extensions/WikibaseLexememaster+41 -51
Replace getMessageObject calls with assertStatusErrormediawiki/extensions/WikibaseLexememaster+22 -40
Replace getMessage(Object) with assertStatusGood/Error etc.mediawiki/extensions/CentralAuthmaster+14 -22
Stop using core's ILocalizedException interfacemediawiki/extensions/FileImportermaster+3 -4
Introduce AbstractLocalizedExceptionmediawiki/coremaster+50 -5
[WIP] Replace ILocalizedException with AbstractLocalizedExceptionmediawiki/extensions/FileImportermaster+46 -12
Customize query in gerrit

Related Objects
Search...

Event Timeline

• Pchelolo created this task.Jul 26 2021, 6:51 PM
Aklapper added a project: Dependency injection.Jul 26 2021, 7:44 PM
Nikerabbit subscribed.Jul 28 2021, 6:53 AM
• Pchelolo assigned this task to tchin.Jul 29 2021, 6:19 PM
daniel subscribed.Aug 26 2021, 3:33 PM

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
taavi subscribed.Sep 9 2021, 7:01 PM
gerritbot added a comment.Sep 9 2021, 8:37 PM

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

[mediawiki/core@master] Introduce AbstractLocalizedException

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

gerritbot added a project: Patch-For-Review.Sep 9 2021, 8:37 PM
gerritbot added a comment.Sep 10 2021, 5:56 PM

Change 720105 merged by jenkins-bot:

[mediawiki/core@master] Introduce AbstractLocalizedException

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

ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.23; 2021-09-13).Sep 10 2021, 6:00 PM
Maintenance_bot removed a project: Patch-For-Review.Sep 10 2021, 6:10 PM
gerritbot added a comment.Sep 13 2021, 8:49 PM

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

gerritbot added a project: Patch-For-Review.Sep 13 2021, 8:49 PM
thiemowmde subscribed.EditedSep 14 2021, 10:33 AM

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.

thiemowmde added projects: Move-Files-To-Commons, WMDE-TechWish-Maintenance, User-thiemowmde.Sep 14 2021, 10:34 AM
tchin added a comment.Sep 14 2021, 6:02 PM

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?

gerritbot added a comment.Sep 14 2021, 6:52 PM

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

Maintenance_bot removed a project: Patch-For-Review.Sep 14 2021, 7:10 PM
Daimona subscribed.Apr 22 2022, 2:13 PM
tchin removed tchin as the assignee of this task.Sep 19 2023, 6:58 AM
tchin subscribed.
gerritbot added a comment.Sep 26 2023, 2:30 PM

Change 961120 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/FileImporter@master] Stop using core's ILocalizedException interface

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

gerritbot added a project: Patch-For-Review.Sep 26 2023, 2:30 PM
gerritbot added a comment.Sep 26 2023, 2:41 PM

Change 961122 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/CentralNotice@master] Stop binding against core's ILocalizedException interface

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

gerritbot added a comment.Sep 26 2023, 3:01 PM

Change 961120 merged by jenkins-bot:

[mediawiki/extensions/FileImporter@master] Stop using core's ILocalizedException interface

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

ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.29; 2023-10-03).Sep 26 2023, 4:00 PM
gerritbot added a comment.Sep 27 2023, 5:56 PM

Change 961436 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] Replace getMessageObject calls with StatusValue::hasMessage

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

gerritbot added a comment.Sep 27 2023, 5:59 PM

Change 961437 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/CentralAuth@master] Replace getMessageObject call with StatusValue::hasMessage

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

gerritbot added a comment.Sep 28 2023, 2:45 PM

Change 961436 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Replace getMessageObject calls with assertStatusError

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

gerritbot added a comment.Sep 28 2023, 2:53 PM

Change 961437 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Replace getMessage(Object) with assertStatusGood/Error etc.

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

gerritbot added a comment.Sep 29 2023, 8:24 PM

Change 962083 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] [WIP] Replace remaining ILocalizedException::getMessageObject calls

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

gerritbot added a comment.Sep 30 2023, 8:35 AM

Change 962111 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/WikibaseLexeme@master] Make use of upstream assertStatusError in tests

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

gerritbot added a comment.Sep 30 2023, 3:54 PM

Change 962111 merged by jenkins-bot:

[mediawiki/extensions/WikibaseLexeme@master] Make use of upstream assertStatusError in tests

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

gerritbot added a comment.Oct 4 2023, 2:21 PM

Change 961122 merged by jenkins-bot:

[mediawiki/extensions/CentralNotice@master] Stop binding against core's ILocalizedException interface

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

ReleaseTaggerBot edited projects, added MW-1.41-notes (1.41.0-wmf.30; 2023-10-10); removed MW-1.41-notes (1.41.0-wmf.29; 2023-10-03).Oct 4 2023, 3:00 PM
thiemowmde moved this task from Incoming to TechDebt on the User-thiemowmde board.Nov 20 2023, 8:55 AM
DannyS712 added a project: MediaWiki-General.Jan 10 2024, 1:31 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL