Page MenuHomePhabricator

Audit and fix extensions that call saveSettings() to lazy-set preferences when loaded
Closed, ResolvedPublic

Description

These can cause writes on GET/HEAD and would break the site in read-only mode where it not for the hacky wfReadOnly() check in saveSettings. They can still break the site on master failure.

Details

Related Gerrit Patches:
mediawiki/extensions/Translate : masterDefer the ApiTranslateUser::trackGroup saveSettings() call
mediawiki/extensions/ContentTranslation : masterDefer the user update in enableCXBetaFeature()

Event Timeline

aaron created this task.Mar 30 2015, 8:44 PM
aaron claimed this task.
aaron raised the priority of this task from to Normal.
aaron updated the task description. (Show Details)
aaron added projects: Epic, MediaWiki-Core-Team.
aaron added subscribers: Gilles, GWicke, mark and 7 others.
Gilles added a comment.Apr 1 2015, 8:48 AM

This looks like something I can do. What's the "right way" for these extensions to behave in regards to their preferences?

aaron added a comment.Apr 16 2015, 4:00 AM

This looks like something I can do. What's the "right way" for these extensions to behave in regards to their preferences?

I see a few anti-patterns:
a) Adding preferences for beta features or the like because a new feature was made and the default was not applied to that user. I don't see why those can't just use application defaults (accounting for the generic opt-in preferences) and log the changes somewhere for analytics. If they really have to update the DB it could be via jobs I suppose (rather than whenever preferences just happen to be loaded).
b) Using hidden "preferences" to track "last visited" data (e.g. NewPagesFeed). These could probably just use AJAX, since that page is JS only anyway. I think Echo and other extensions may do this kind of thing to...maybe the same applies there.

Also, one related thing to do is to add wfReadOnly() checks there since some callers seem OK with doing nothing were as other don't, and I can't change User until the callers are updated.

aaron moved this task from Backlog to Doing on the Availability board.Apr 27 2015, 8:47 PM
aaron moved this task from Doing to Backlog on the Availability board.

I think "last-sceen" type things should use the object stash instead of User.

Logs still show:

SpecialContentTranslation.php line 90 calls SpecialContentTranslation->enableCXBetaFeature()

Change 240022 had a related patch set uploaded (by Aaron Schulz):
Defer the user update in enableCXBetaFeature()

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

Change 240022 merged by jenkins-bot:
Defer the user update in enableCXBetaFeature()

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

Change 240035 had a related patch set uploaded (by Aaron Schulz):
Defer the ApiTranslateUser::trackGroup saveSettings() call

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

Change 240035 merged by jenkins-bot:
Defer the ApiTranslateUser::trackGroup saveSettings() call

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

aaron closed this task as Resolved.Sep 22 2015, 6:59 AM

That takes care of the things spamming the logs.