Page MenuHomePhabricator

Change EchoEvent->create to return Status or throw exceptions, never false
Open, Needs TriagePublic

Description

It's impossible to determine which error occurred if it just returns false for multiple things.

I think they should all throw exceptions (programmer error) (including the serialization error which is a validation/insufficient truncation error in whatever feeds into the notification's extra data) except:

  • DB read-only (maybe better to lose the notification than blow up the request with an exception? This generally shouldn't happen since whatever *causes* the notification should also not happen due to read-only)
  • BeforeEchoEventInsert (I think suppressing the notification silently is intended here)

Those two should return a Status.

This is a breaking change and any checks for this in Wikimedia-deployed code also need to be fixed, and it needs to be documented in release notes as a breaking change.

Event Timeline

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptJul 5 2017, 11:32 PM
Restricted Application added subscribers: TerraCodes, Aklapper. · View Herald Transcript
Mattflaschen-WMF updated the task description. (Show Details)
Mattflaschen-WMF updated the task description. (Show Details)
Mattflaschen-WMF updated the task description. (Show Details)
Mattflaschen-WMF updated the task description. (Show Details)

Change 371505 had a related patch set uploaded (by JoHammer; owner: JoHammer):
[mediawiki/extensions/Echo@master] Change EchoEvent->create to return status or throw exceptions

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

Framawiki moved this task from Backlog to Doing on the good first bug board.Dec 2 2017, 1:29 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptSep 24 2018, 9:57 AM
SBisson moved this task from Inbox to Triaged but Future on the Growth-Team board.Sep 28 2018, 2:27 AM