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

Assigned To

None

Authored By

	• Mattflaschen-WMF
	Jul 5 2017, 11:32 PM

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.

Details

	Subject	Repo	Branch	Lines +/-
	Add EchoEvent::createEvent, returns status on failure	mediawiki/extensions/Echo	master	+38 -7
	Change EchoEvent->create to return status or throw exceptions	mediawiki/extensions/Echo	master	+6 -4

Customize query in gerrit

Related Objects
Search...

Status	Subtype	Assigned	Task
Stalled	Feature	None	T235873 Add variables in Abusefilter to detect/block thanks
Open	BUG REPORT	None	T241474 Thank api assumes that echo event will go through
Open		None	T169834 Change EchoEvent->create to return Status or throw exceptions, never false

Event Timeline

• Mattflaschen-WMF created this task.Jul 5 2017, 11:32 PM

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)Jul 5 2017, 11:33 PM

• Mattflaschen-WMF updated the task description. (Show Details)

• Mattflaschen-WMF updated the task description. (Show Details)Jul 5 2017, 11:36 PM

• Mattflaschen-WMF updated the task description. (Show Details)

• jmatazzoni moved this task from Untriaged to Medium-Term Interest on the Collaboration-Team-Triage board.Jul 12 2017, 8:45 PM

JoHammer claimed this task.Aug 10 2017, 10:08 PM

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

gerritbot added a project: Patch-For-Review.Aug 11 2017, 5:36 PM

Framawiki moved this task from Backlog to Doing on the good first task board.Dec 2 2017, 1:29 PM

• Phabricator_maintenance moved this task from Doing to Backlog on the good first task board.Sep 24 2018, 9:57 AM

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

Liuxinyu970226 removed a project: Collaboration-Team-Triage.Oct 4 2018, 4:23 AM

Liuxinyu970226 subscribed.

matej_suchanek mentioned this in T242153: EchoEvent::create should allow returning a status object.Jan 9 2020, 8:39 AM

DannyS712 merged a task: T242153: EchoEvent::create should allow returning a status object.Jan 9 2020, 6:14 PM

DannyS712 added a parent task: T241474: Thank api assumes that echo event will go through.

DannyS712 added subscribers: DannyS712, matej_suchanek.

TerraCodes unsubscribed.Jan 10 2020, 3:08 AM

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

@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!

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

Restricted Application added a project: User-DannyS712. · View Herald TranscriptFeb 12 2020, 8:38 AM

DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.Feb 12 2020, 8:38 AM

Is growth team willing to review a patch?

Sure :)

DannyS712 removed a project: Patch-Needs-Improvement.Feb 13 2020, 12:53 AM

In T169834#5875526, @kostajh wrote:

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?

That sounds OK to me, thanks @DannyS712

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

gerritbot added a project: Patch-For-Review.Jun 6 2020, 5:10 AM

In T169834#6014726, @kostajh wrote:

That sounds OK to me, thanks @DannyS712

Sorry for the delay. Patch ready for review

MBinder_WMF added a project: Growth-Team-Filtering.Apr 15 2021, 6:56 PM

Aklapper edited projects, added Patch-Needs-Improvement; removed Patch-For-Review.Feb 21 2023, 10:20 PM

Aklapper removed a subscriber: • Mattflaschen-WMF.

@DannyS712: Removing task assignee as this open task has been assigned for more than two years - See the email sent to task assignee on Feburary 22nd, 2023.
Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome! :)
If this task has been resolved in the meantime, or should not be worked on by anybody ("declined"), please update its task status via "Add Action… 🡒 Change Status".
Also see https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator. Thanks!

Change EchoEvent->create to return Status or throw exceptions, never falseOpen, Needs TriagePublicActions

Description

Details

Related ObjectsSearch...

Event Timeline

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

Related Objects
Search...