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 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)

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

@JoHammer any updates? Are you still working on this?

Aklapper edited projects, added Patch-Needs-Improvement; removed Patch-For-Review.
Aklapper added a subscriber: JoHammer.

@JoHammer: I am resetting the assignee of this task because there has not been progress lately (please correct me if I am wrong!).
Resetting the assignee avoids the impression that somebody is already working on this task. It also allows others to potentially work towards fixing this task.
Please claim this task again when you plan to work on it (via Add Action...Assign / Claim in the dropdown menu) - it would be welcome! Thanks for your understanding!

DannyS712 moved this task from Triaged but Future to Inbox on the Growth-Team board.

Outside of Echo itself, it seems only Flow's NotificationController cares about what the function returns.
Is growth team willing to review a patch?

Is growth team willing to review a patch?

Sure :)

Is growth team willing to review a patch?

Sure :)

Took a look at doing this. Given that the extension is used outside of WMF deploy, and that this would be a breaking change, and that the method currently returns multiple possible exceptions, I'd like to propose:

  • Creating EchoEvent::createEvent to replace ::create
  • Returning a fatal status instead of throwing an exception
  • Still supporting the deprecated EchoEvent::create for 1 release

Thoughts?

Change 602793 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/Echo@master] Add EchoEvent::createEvent, returns status on failure

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

That sounds OK to me, thanks @DannyS712

Sorry for the delay. Patch ready for review