Page MenuHomePhabricator

Regression: UserSaveOptions hook no longer exposes old preference values
Closed, ResolvedPublic

Description

A common pattern for extensions using the UserSaveOptions hook is to detect preference changes by creating a second User object to obtain the old (pre-modification) preference values, like this:

public static function onUserSaveOptions( $user, &$options ) {
	$oldUser = User::newFromName( $user->getName() ); // or User::newFromId( $user>getId() )
	$oldValue = $oldUser->getOption( 'some-preference' );
	$newValue = $options['some-preference'];
	if ( $oldValue !== $newValue ) {
		// The value of 'some-preference' changed, do something special
	}
}

This trick used to work because the to-be-saved preference values were stored only in the User object, and so a newly created User object would not see them.

However, rMW788331c48a57: Introduce UserOptionsManager and DefaultOptionsManager (for T248527: Create UserOptionsManager, deployed in 1.35.0-wmf.31) broke this trick: the to-be-saved preference values are now stored in the UserOptionsManager, and there's only one of those for each user, so creating a fresh User object doesn't circumvent it. This means that extensions using this trick no longer detect most(*) preference changes, which causes all sorts of problems.

This trick is used in at least four places that I could find:

  • In WikimediaEvents, to send events to the PrefUpdate schema, which has dried up(*) as a consequence; see T253151
  • In BetaFeatures, to detect when a beta feature is turned on or turned off and update the user counts for each beta feature; presumably these counts are no longer being updated
  • In Flow, to detect when a certain preference is turned on or turned off; this was reported broken at T234241
  • In GrowthExperiments, to detect when a certain preference is turned on; this was reported broken at T253144

I don't know how feasible it is to make the old behavior work again (User objects don't know about pending unsaved preference changes until they're saved, except for the User object saving them), but alternatively, the old options could be passed to this hook (or a different hook) explicitly. That would still be a breaking change, but it would also be a bit cleaner than the trick we're currently using.

(*) It appears that the only changes that are still being detected are those made by other extensions through this hook (by modifying the $options parameter). This is why PrefUpdate has been recording very few events since wmf.31, but not zero.

Event Timeline

I don't know how feasible it is to make the old behavior work again (User objects don't know about pending unsaved preference changes until they're saved, except for the User object saving them), but alternatively, the old options could be passed to this hook (or a different hook) explicitly. That would still be a breaking change, but it would also be a bit cleaner than the trick we're currently using.

At a quick glance, that seems entirely feasible, and a good solution. A few lines after the hook is triggered in saveOptions(), the old options are loaded from the database. It shouldn't be terribly difficult to do that first, and pass the old values (or a diff?) to the hook.

However, while adding a parameter the a hook signature is not a breaking change conceptually, it has now become a breaking change with the introduction of hook interfaces (i.e. UserSaveOptionsHook). As long as no extension actually implements these interfaces, and the interfaces haven't been in any stable release yet, we can still make such a change.

Change 598826 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Add $originalOptions parameter to UserSaveOptions hook

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

Change 598837 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/BetaFeatures@master] Use UserSaveOptions $originalOptions parameter

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

The two patches above introduce the new hook parameter and demonstrate it's usage in an extension. I will make patches to the rest of the extensions once the review of the core patch is completed and we're in agreement on the API.

Change 598826 merged by jenkins-bot:
[mediawiki/core@master] Add $originalOptions parameter to UserSaveOptions hook

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

Change 599373 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/WikimediaEvents@master] Use $originalOptions in UserSaveOptions hook

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

Change 599376 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/GrowthExperiments@master] Use $originalOptions in UserSaveOptions hook

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

Change 599377 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/Flow@master] Use $originalOptions in UserSaveOptions hook

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

Change 598837 merged by jenkins-bot:
[mediawiki/extensions/BetaFeatures@master] Use UserSaveOptions $originalOptions parameter

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

Change 599373 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] Use $originalOptions in UserSaveOptions hook

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

Change 599376 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Use $originalOptions in UserSaveOptions hook

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

Change 599377 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Use $originalOptions in UserSaveOptions hook

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