Page MenuHomePhabricator

Put RCFilters i18n messages and other config in a ResourceLoader data module
Open, Needs TriagePublic

Description

As part of T192623: ResourceLoader 2018 Review, I noticed that RCFilters exports i18n messages by embedding them into the page in an inline script tag. This is fragile and is about to become more fragile, so I'm moving the message blob into mw.config. But that just moves it to a different inline script tag, and on enwiki this blob is 9 KB. On top of that, the other config blob for RCFilters (wgStructuredChangeFilters) containing the main filters config is 12.5 KB. The tag list (wgRCFiltersChangeTags) is another 19.2 KB.

Instead of embedding this 40 KB of data into the page and shipping it on each page load, we should load it as an RL module that is only invalidated when the data changes. In order to be able to do this, we would have to solve the following problems.

Conditional filter registration: Right now, filters that are available conditionally are registered conditionally. That means that, if the user is not allowed to use the patrol filter, that filter is never registered and the system doesn't even know it exists (for the purposes of that request). An RL data module does not have access to the main request context, and can't vary its output based on who the user is or what the query string parameters are. Instead, these kinds of filters would have to always be registered, and have a property with a callback determining whether they're available or unavailable for a given request. This information would then have to be passed down to the client.

Currently, the following filters are registered conditionally:

  • The reviewStatus and legacyReviewStatus groups are conditional on the user being allowed to patrol, and the special page not being in transclusion mode. Both of these are request-specific, so they aren't OK.
  • The hideCategorization filter is conditional on $wgRCWatchCategoryMembership. This is fine, because it's based on config, not the specific request.
  • The watchlist group (RC-only) is conditional on the user being logged in and having the viewmywatchlist right, and on not being in transclusion mode. None of these are OK.
  • ORES filters are conditional on a bunch of things, but they're all config or values periodically being fetched from a service, and none of them are request specific. This is fine.
  • Wikibase registers its filters conditional on config (OK) and registers a conflict conditional on ORES being present (OK)

Filter definitions contain request-dependent data: Many filters have default values that depend on user preferences. ORES filters also have default highlights that depend on user preferences. To tackle these, we could introduce a new filter definition property to express preference-based defaults declaratively, and handle default highlights either with a callback or on the client. The only preference-based defaults I found that are nontrivial to express are the userExpLevel group's default depending on both the hideliu and hideanon preferences, and the hidepreviousrevisions default on WL being the inverse of a preference.

Filter availability and definitions vary between RC and WL: ChangesListSpecialPage::transformFilterDefinition() is overridden by each subclass to compute showHide from showHideSuffix where needed, but I don't think that affects filter definitions that get sent to the client. Both RC and WL register filters that only exist on one and not the other: the watchlist group is RC-only, and the extended-group and watchlistactivity groups are WL-only. Some definition properties vary as well. The names of the preferences controlling defaults vary between RC and WL for almost all filters, and a few filters have preference-controlled defaults only on one of the two. We could solve these issues by marking filters/groups as RC-only or WL-only where appropriate, and by having separate rcdefault and wldefault properties.

Filter definitions are not fully declarative: they mostly are, but to register subsets and conflicts you need to call methods on the filter objects. This is not a big problem in and of itself (an RL data module could run the code needed to build the tree of objects, then serialize it as we do now), but it's linked to the problem that you need a ChangesListSpecialPage instance to get to a fully-built filter registry. If the definitions are fully declarative, we can just take them and serialize them without needing to run much code (we'd still need to run hooks that extensions use to add filters, or migrate this to an extension.json attribute) and without needing to instantiate a UI class in a place where we're not supposed to be using the request context.

What we need to solve to pull out what:

  • Tag list: Nothing, this can be pulled out already
  • Messages: Conditional registration, RC vs WL differences (need to be able to discover all messages that could possibly be used), full declarativeness (conflicts include messages)
  • FIlter data: Everything

Related Objects

Event Timeline

Change 460954 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] RCFilters: Export config vars in the RL module where possible

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

Change 460954 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] RCFilters: Export config vars in the RL module where possible

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

This patch does the tag list part, but does not do the bulk of the task.

kostajh edited projects, added Growth-Team; removed Growth-Team (Current Sprint).
kostajh added a subscriber: kostajh.

@Catrope moving this back to In Progress, then.

Change 460954 merged by jenkins-bot:
[mediawiki/core@master] RCFilters: Export config vars in the RL modules where possible

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

Change 460954 merged by jenkins-bot:
[mediawiki/core@master] RCFilters: Export config vars in the RL modules where possible

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

Reverted in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/489927 : an interaction with the OAuth extension caused this to throw Sessions are disabled for this entry point errors

Change 490958 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/extensions/OAuth@master] Respect language code in onMessagesPreLoad hook

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

Change 490935 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] Revert "Revert "RCFilters: Export config vars in the RL modules where possible""

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

Change 490958 merged by jenkins-bot:
[mediawiki/extensions/OAuth@master] Respect language code in onMessagesPreLoad hook

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

Change 490935 merged by jenkins-bot:
[mediawiki/core@master] RCFilters: Export config vars in the RL modules where possible (take 2)

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

Kicking this back out of the sprint now that the short-term part of this (the tag list) is done.

Change 516850 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] [WIP] RCFilters: Reduce startup overhead from 'config.json' computation

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

Change 516850 merged by jenkins-bot:
[mediawiki/core@master] RCFilters: Reduce startup overhead from 'config.json' computation

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

@Catrope do you want to unassign yourself from this if you don't plan to work on it in the near term? Do you think we should schedule this for Q1, Q2, or farther out?