Page MenuHomePhabricator

Notifications should be in core
Open, Stalled, MediumPublic

Assigned To
None
Authored By
Legoktm
Feb 29 2016, 7:25 AM
Referenced Files
None
Tokens
"Love" token, awarded by MGChecker."Like" token, awarded by Liuxinyu970226."Barnstar" token, awarded by Zppix."Love" token, awarded by matej_suchanek."Barnstar" token, awarded by daniel."Dislike" token, awarded by Ricordisamoa."Like" token, awarded by Luke081515."Like" token, awarded by Bawolff."Like" token, awarded by Anomie."Like" token, awarded by fbstj."Like" token, awarded by Krenair."Like" token, awarded by Florian."Like" token, awarded by Jdforrester-WMF.

Description

  • Affected components: MediaWiki core (Watchlist notification, Talk page notif, Password reminder etc.), Echo extension, LoginNotify extension.
  • Engineer(s) or team for initial implementation: TBD.
  • Code steward: TBD.

Motivation

MediaWiki's notification system, implemented as a result of the Notification framework RFC, currently lives in the Echo extension. It should be in MediaWiki core.

  • A notification system is fundamental to any kind of modern website and should be part of the default MediaWiki experience.
  • Being in an extension, Echo has to use ugly hooks to suppress some core features and has generally weird interactions with some things.
  • extensions that wish to use it have to do conditional checks every time they'd like to trigger a notifications.
  • Echo is a reserved word in PHP, preventing us from using it as the namespace for a PHP class. ;-)
Requirements

(TBD: Specify the features or criteria that need to be met.)


Exploration

(TBD: Use this space for data gathering, status quo, proposals, other considerations etc.)

Former RFC: Notifications RFC (2011)

There was an RfC on this topic in 2011, without any definite outcome. See the comments of this task, mw:Requests_for_comment/Notifications_in_core, and the IRC logs P3638 (wmflabs archive) and P3640 (wmflabs archive).

Former RFC: Merge Notifications and Watchlist (2014)

See T128352: RfC: Need to merge Notifications and Watchlist or lack thereof.

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Quick note on my position after IRC discussion:

Having the interfaces available in core is I think very important for establishing server->user notifications as a standard part of the MediaWiki platform which can be reliably used by features both in core and extensions. For this reason I tend to think that decent email-based and on-web notification view implementations are also required in core, or the system can't be relied upon to actually work.

Thus I tend to advocate for a fairly feature-complete merge over an internal-API-only merge with all implementation done separately in the extension (even if it's bundled in the tarball, if it's not always enabled it can't be relied on as part of the platform).

I think the simplest option is to:

  • Bundle Echo in the tarball for a release (it's a popular extension already, but it doesn't hurt to do this to demonstrate that make sure sysadmins widely use it before it's in core).
  • Merge Echo into core piece by piece.

If I understand correctly, Krinkle's interface proposal is roughly:

  1. Move some Echo email logic to core.
  2. Refactor core's existing enotif code to provide a wgNotificationHandler.
  3. Have Echo's email class subclass core's email one. Echo would provide its own wgNotificationHandler with more sophisticated functionality, including a web UI (popup/flyout and special page), plus bundling and better email.

For Echo to really reuse core's email notification functionality (to not have two implementations), core would have to change to use Echo's email notification formatting. At that point (Echo email's been brought into core), why not also bring the Echo web notifications to core?

It would be nice if MediaWiki was built as a set of loosely-coupled components (such as, say, Symfony2 where messages, resource loading, pages etc. are separate components), and in such a world making notifications a separate component would make perfect sense. We are not in that world, and extensions are not components: they exist to add or replace functionality, not to package default functionality needed by everyone. There is a maintenance burden to extensions which replace core functionality (patches must be split up and are more complicated to review and deploy; relevant code is harder to find; CI setup must be duplicated); I don't see what we would gain here by paying that burden.

MW core is expected to be marginally useful even without extensions; I wouldn't consider it reasonable to have a core where e.g. you ask for a password reminder and nothing happens because the notification went to /dev/null. So email notification support should be in core, and at least the concepts for web notifications need to be in core (things like read/unread and bundling and message expiration need to be part of the interface); it would be possible to put the interface for web notifications in core, and leave the implementation in Echo, but again that seems like maintenance cost (every change had to be coordinated in two places) with unclear benefits.

RobLa-WMF mentioned this in Unknown Object (Event).May 11 2016, 12:09 AM

To me it seems like we should at least have the interfaces (aka, the services) an a basic (or empty) implementation of a notification system in core. This mainly means replacing the hook handlers that Echo uses to hook into core with proper service interfaces that all core components can know about and use.

Current state:

Me & Timo are in agreement that we need good enough interfaces in core that other extensions don't have to depend on Echo; also agreed that shipping Echo with core as a default extension makes sense.

We still have to discuss the 'line' of how much of Echo infrastructure needs to be moved to core, which'll be the subject of next week's archcom IRC meeting. :)

@brion @Krinkle do you think you could propose strawman service interfaces to replace Echo hook handlers, for next week's discussion? These would be the "good enough interfaces", I suppose.

We still have to discuss the 'line' of how much of Echo infrastructure needs to be moved to core, which'll be the subject of next week's archcom IRC meeting. :)

/me nods in agreement

We have this planned for next week's IRC meeting:
E251: ArchCom RFC Meeting W31: Notifications in core (2016-08-03, #wikimedia-office)

I'm not sure how realistic it is to find an abstraction that is sufficiently non-leaky that it's worth separating from Echo. The current interface is basically EchoEventPresentationModel (ie. getType, getCategory, getIconType, canRender, getHeaderMessage, getCompactHeaderMessage, getSubjectMessage, getBodyMessage getPrimaryLink, getSecondaryLinks, plus something for bundling support if that should be part of core, plus something for finding the target users) and that very much depends on the current Echo flyout design. An alternative implementation with a different design probably would not find the body / primary links / secondary links split that useful.

I'm not sure how realistic it is to find an abstraction that is sufficiently non-leaky that it's worth separating from Echo. The current interface is basically EchoEventPresentationModel [details on that] An alternative implementation with a different design probably would not find the body / primary links / secondary links split that useful.

Let me see if I understand this based on a naive reading of what you wrote. When a server application wants to present a message to a user, it instantiates an object that implements the EchoEventPresentationModel interface, and puts that object in Echo's message queue somehow. Do I have that basically right?

Currently (when using the non-legacy method), you would

  • use the BeforeCreateEchoEvent hook to register event types and set up some other minor details such as icon URLs
  • register an EchoEventPresentationModel for each event type, which can convert an EchoEvent into a predefined set of messages (header, body etc)
  • when the event happens, instantiate and queue an EchoEvent
  • use the EchoGetDefaultNotifiedUsers hook to return a list of users to notify

EchoEvent and EchoGetDefaultNotifiedUsers lend themselves to standardization well; the first is basically just a hashmap of event details, the second is just a service that gets an EchoEvent and has to return a user list. EchoEventPresentationModel seems hard to untangle from implementation details. Or you can just leave it out, in which case notification-sending extensions would have a standard event model but would have to define a presentation model for each implementation they want to be compatible with.

Things might get even more complex if you consider bundling ("your article was linked 10 times!" instead of ten separate "your article was linked by X!" messages), but I never actually used that so not sure.

An alternative implementation with a different design probably would not find the body / primary links / secondary links split that useful.

Even if it's not useful, dealing with that split doesn't seem at all difficult. The split seems fairly generic to me: you have content, a main link, and possibly additional links.

Bundling, on the other hand, might be difficult for some implementations to deal with unless the bundle-event's primary link is to a page that lists the subevents. From the notification-generation side it can probably just ignore bundling, while a consumer that can't support it would probably want to either unbundle or request unbundled events.

The split seems fairly generic to me: you have content, a main link, and possibly additional links.

Echo requires you to not have any links in your content (instead the primary link will wrap the content). A different implementation might allow links inside and not need the primary link. Or it might want a short and full version of the notification, for a dense list with show/hide. Or it might want a guranteed <140 characters message for SMS notifs. Etc. Hard to be generic, and seems like a waste of time to me given how small the chance is that someone would write an Echo alternative.

No links in the body seems like a reasonable restriction to me. A short
(<140 char?) summary is something that could certainly be added, although
yagni may apply.

Hard to be generic, and seems like a waste of time to me given how small the chance is that someone would write an Echo alternative.

I can respect a desire not to generalize something that seems to be working well enough for the current usage patterns. Just to clarify: do you believe that Echo implements essential notification functionality that belongs in core, or do you think having it as an extension (and implicitly optional) is fine? My reading of your March comment (T128351#2105216) is that you're supportive of moving Echo notifications into core.

My impression is that the functionality implemented by Echo should be defined by core (not necessarily implemented). But the code currently in the Echo extension apparently isn't fit for that. Defining a proper notification interface from scratch may be easier than trying to merge (part of) Echo in.

do you believe that Echo implements essential notification functionality that belongs in core

I do believe that. And I am usually a fan of unnecessary generalizations :) I just think that view interfaces are very hard to generalize in a useful way, and more so when trying to generalize from a sample set of 1.
(I am not familiar with the codebase beyond what is exposed to clients of the extension, so I have no opinion on whether it is a bad idea to merge the code as it is.)

Defining a proper notification interface from scratch may be easier than trying to merge (part of) Echo in.

I just think that view interfaces are very hard to generalize in a useful way, and more so when trying to generalize from a sample set of 1.

@Tgr makes a very powerful point here. Generalizing "from a sample set of 1" seems really dangerous. @daniel, you seem to be proposing something even more dangerous: design something "from scratch". What problem does that solve? Who would do this work? If we don't use Echo functionality, how would we integrate Echo with it?

@RobLa-WMF: I'm not sure what @Tgr means by a view-interface in this context. I agree for UI design - it's very hard to do that in a generic way.

Abstracting from a sample set of one is of course also very hard - but that's not what I propose. Echo is already an abstraction of various kinds of notifications we need. I propose to define an actual service interface for this functionality, instead of relying on hooks, which we would do if we just merge Echo into core as-is.

Generally, hooks are not used within core; so when moving Echo functionality into core, it should be re-defined in terms of an actual service interface. Besides the general advantages of a service interface, such as type-safety, a service interface clearly defines what notification functionality is available, and lets core code interact with it directly.

My point is: core code should not listen e.g. to the LinksUpdateAfterInsert, it should rather be całled directly by the LinksUpdate class. That is much more obvious and safer.

To me, the crucial part of this RFC is defining this interface that core can call to trigger notifications (or, to put the the other way around, the interface core offers to trigger notifications).

As a point-in-case, here are all the hooks that Echo registers:

	// Housekeeping hooks
	$wgHooks['LoadExtensionSchemaUpdates'][] = 'EchoHooks::onLoadExtensionSchemaUpdates';
	$wgHooks['GetPreferences'][] = 'EchoHooks::getPreferences';
	$wgHooks['PersonalUrls'][] = 'EchoHooks::onPersonalUrls';
	$wgHooks['BeforePageDisplay'][] = 'EchoHooks::beforePageDisplay';
	$wgHooks['MakeGlobalVariablesScript'][] = 'EchoHooks::makeGlobalVariablesScript';
	$wgHooks['UnitTestsList'][] = 'EchoHooks::getUnitTests';
	$wgHooks['ResourceLoaderRegisterModules'][] = 'EchoHooks::onResourceLoaderRegisterModules';
	$wgHooks['EventLoggingRegisterSchemas'][] = 'EchoHooks::onEventLoggingRegisterSchemas';
	$wgHooks['ResourceLoaderTestModules'][] = 'EchoHooks::onResourceLoaderTestModules';
	$wgHooks['UserGroupsChanged'][] = 'EchoHooks::onUserGroupsChanged';
	$wgHooks['UserLoadOptions'][] = 'EchoHooks::onUserLoadOptions';
	$wgHooks['UserSaveOptions'][] = 'EchoHooks::onUserSaveOptions';
	$wgHooks['UserClearNewTalkNotification'][] = 'EchoHooks::onUserClearNewTalkNotification';
	$wgHooks['ParserTestTables'][] = 'EchoHooks::onParserTestTables';
	$wgHooks['EmailUserComplete'][] = 'EchoHooks::onEmailUserComplete';
	$wgHooks['LoginFormValidErrorMessages'][] = 'EchoHooks::onLoginFormValidErrorMessages';

	// Extension:UserMerge support
	$wgHooks['UserMergeAccountFields'][] = 'EchoHooks::onUserMergeAccountFields';
	$wgHooks['MergeAccountFromTo'][] = 'EchoHooks::onMergeAccountFromTo';
	$wgHooks['UserMergeAccountDeleteTables'][] = 'EchoHooks::onUserMergeAccountDeleteTables';

	// Extension initialization
	$wgExtensionFunctions[] = 'EchoHooks::initEchoExtension';

	require __DIR__ . '/Resources.php';

	$wgHooks['EchoGetBundleRules'][] = 'EchoHooks::onEchoGetBundleRules';
	$wgHooks['EchoAbortEmailNotification'][] = 'EchoHooks::onEchoAbortEmailNotification';

	// Hook appropriate events
	$wgHooks['ArticleSaveComplete'][] = 'EchoHooks::onArticleSaved';
	$wgHooks['AddNewAccount'][] = 'EchoHooks::onAccountCreated';
	$wgHooks['ArticleRollbackComplete'][] = 'EchoHooks::onRollbackComplete';
	$wgHooks['UserSaveSettings'][] = 'EchoHooks::onUserSaveSettings';

	// Disable ordinary user talk page email notifications
	$wgHooks['AbortTalkPageEmailNotification'][] = 'EchoHooks::onAbortTalkPageEmailNotification';
	$wgHooks['SendWatchlistEmailNotification'][] = 'EchoHooks::onSendWatchlistEmailNotification';
	// Disable the orange bar of death
	$wgHooks['GetNewMessagesAlert'][] = 'EchoHooks::abortNewMessagesAlert';
	$wgHooks['LinksUpdateAfterInsert'][] = 'EchoHooks::onLinksUpdateAfterInsert';

	// Beta features
	$wgHooks['GetBetaFeaturePreferences'][] = 'EchoHooks::getBetaFeaturePreferences';

	// Global config vars
	$wgHooks['ResourceLoaderGetConfigVars'][] = 'EchoHooks::onResourceLoaderGetConfigVars';

Only the hooks in the Hook appropriate events section would need to be covered by the notification service interface.
The functionality of the house keeping hooks would probably be baked into core inline.

So, my point was: it may be simpler to think up a simple notification service interface, and then implement that interface based on existing code, then to do it the other way around, and attemt to model everything Echo does into services.

My point is: core code should not listen e.g. to the LinksUpdateAfterInsert, it should rather be całled directly by the LinksUpdate class. That is much more obvious and safer.

Well yes, no one is proposing to do that.

Well yes, no one is proposing to do that.

Excellent. Then let's discuss what to do instead. What interface should core be using for notifications?

Brief clarification from the ArchCom meeting (thanks, @Krinkle):

Merging Echo "as-is" still means getting rid of hooks, which is a non-trivial refactoring task.

Merging "as-is" not really a light weight option. The proposal is rather to integrate Echo's data model and UI into core as-is, rather than re-engineering it. But integration with the rest of core will have to be re-engineered in any case.

The proposal is not to move the extension into a directory in core and hook it up like an extension.

  • use the EchoGetDefaultNotifiedUsers hook to return a list of users to notify

Note, this is also deprecated. The recommend way to do that is now EchoAttributeManager::ATTR_LOCATORS / 'user-locators' , e.g. https://phabricator.wikimedia.org/diffusion/EFLW/browse/master/includes/Notifications/Notifications.php;e90b910f3720d1f932ec83a15e40f3ca94c08068$101 .

Quick brain dump after the IRC meeting:

  • there is logic that is specific to each event type - in particular detecting the event, and deciding who should be notified (user location, aka routing). Which event types should be implemented in core is still unclear. Maybe everything that Echo covers, maybe not. * Extensions must be able to define new event types.
  • there is logic that is common to all even types ("infrastructure" for registering even types, triggering relevant callbacks)
  • delivery/presentation logic (email, popups, whatever) should be orthogonal to event types. Core should provide a decent implementation (probably not just email).
  • many delivery mechanisms need some sort of storage. It's unclear if core can provide a generic storage facility for this purpose, or whether the needs of the various delivery mechanisms are too different.
  • an essential component will be a service interface for posting (Echo)Event objects for eventual delivery, with user location / routing hidden.
    • a more low level service would take care of delivering a specific message to a specific user
  • It's unclear to me how event detection can be generalized. Extensions should probably rely on hook handlers, and core can just have the detection code inlined. I can't imagine a general event detection interface.

So, there are essentially two discussions

  • what infrastructure should be in core, what should its API(s) look like.
    • e.g. Buble popup delivery seems uncontroversial, storage is unclear.
  • which event types should be supported in core?

IMO it's more helpful to think of it in terms of event triggering instead of event detection. Echo has two parts: a generic event handling framework with interfaces for triggering, routing and presenting events; and event-specific code that hooks into core to trigger events and hooks into the event framework to tell it what to do with them. There is not much point in trying to generalize the second part.

Current state:

Me & Timo are in agreement that we need good enough interfaces in core that other extensions don't have to depend on Echo; also agreed that shipping Echo with core as a default extension makes sense.

Let's try to define what a "good enough interface" means. Specifically, what capabilities should be standardised in such a way that core and any extension can express those without depending on Echo.

Basic interface

  • Register an event type (by canonical name and perhaps by class).
  • Instantiate an event object of a certain type, with a set of standardised required and/or optional parameters associated a given event type. (Accessed via an abstract class interface defining getter methods).
  • Getting a hold of the event queue.
  • Emitting an event object.
  • Listening to events.

This is not unlike Job, JobQueue and JobRunner. $wgJobClasses, $wgJobTypeConf (JobQueueDB), $JobQueueAggregatorNull. It's also somewhat similar to our EventRelayer interface.

Default handler

After the basics, another major decision to make is what the default core behaviour should be if no extensions are installed.

For most internal interfaces we tend to default to no-op (e.g. NullRelayer, NullLogger). This may be acceptable for notifications, but I can also see arguments to the contrary. When an extension queues a Job, it expects that work to be executed. Just like it would execute outside a job. Similarly, when an extension adopts the notifications interface (instead of automating a talk-page message or user-mail), it will expect that message to be delivered regardless of the site's configuration and installed extensions.

Following that logic, I expect we'd implement a very basic handler. Maybe it'd simply enqueue a job that mails the user, perhaps with a digest preference.

It seems a waste to demand more of the default handler as it would be similar to Echo but mostly unused. I think there's definitely room for innovative Echo alternative developed by third parties. And when that happens, people will have a choice. The reason for the default is just to make sure that extensions don't have to think about a scenario where messages are never deliverable.

We also need to decide whether a distinction between notifications to web vs mail should be standardised or left as implementation detail. For most cases I'd say this is an implementation detail. However there is one interesting exception: Notifications for when a user e-mails you. Obviously we'd don't want to notify users by e-mail about another e-mail. Though this could be mitigated by simply having the default preference be turned off for that one.

Built-in events

Just implementing the abstract interface and default handlers will solve the problem that extensions need to hardcode Echo extension references. But it won't change the fact that Echo needs a lot of hooks.

There are two types of core events that we'll probably want to do:

  1. Override core behaviour: Right now, MediaWiki does have certain notification abilities already (new talk page message, watchlist etc.). These would now become the first notifications and by default e-mailed through the default event handler (instead of the hard-coded mail sending logic for those, which Echo disables). When Echo is installed, these notifications would naturally be handled by Echo instead (e.g. sent to both web and mail, and a different mail format).
  2. New behaviour: Notifications for user group changes, mentions, reverts.

These would presumably by emitted by core instead of via hooks in Echo. Though extensions and site admins should be able to disable them by setting their class handler to false in the event type registry.

  1. Override core behaviour: Right now, MediaWiki does have certain notification abilities already (new talk page message, watchlist etc.).

Watchlist is a complicated issue. We may not want that to use core-ified Echo. See T128352: RfC: Need to merge Notifications and Watchlist or lack thereof.

de-assigning, not active on this

Are there any plans to move forward with this?

Closing old RFC that is not yet on to our 2020 process and does not appear to have an active owner. Feel free to re-open with our template or file a new one when that changes.

Jdforrester-WMF changed the task status from Declined to Invalid.Sep 16 2020, 8:09 PM

Not Declined.

Tgr renamed this task from RfC: Notifications in core to Notifications should be in core.Sep 17 2020, 3:29 AM
Tgr reopened this task as Open.
Tgr lowered the priority of this task from High to Medium.
Tgr edited projects, added Proposal; removed TechCom-RFC.
Tgr updated the task description. (Show Details)

This still makes perfect sense as a proposal so disentangling it from TechCom's process tracking choices.

Krinkle changed the task status from Open to Stalled.Sep 18 2020, 2:54 AM
Krinkle edited projects, added TechCom-RFC; removed Proposal.

If Growth indeed still has intentions to work on this and that further conversation by others would not be futile/ignored, then this is no bother to have on the board. I was just clearing out a backlog.