Not a bug XF\Repository\UsernameChange::clearPendingUsernameChanges doesn't appear to work

[Xon]

Affected version

2.2.1

The call sites for clearPendingUsernameChanges have comments suggesting the cleanup is triggered/done by the function.

class User {
...
// if user has a pending username change then handle them
$usernameChangeRepo->clearPendingUsernameChanges($this);

class UsernameChange {
...
// remove/reject any other pending changes
$this->getUsernameChangeRepo()->clearPendingUsernameChanges($this->User, $this);

But in clearPendingUsernameChanges()

public function clearPendingUsernameChanges(
   \XF\Entity\User $user, \XF\Entity\UsernameChange $skipChange = null, \XF\Entity\User $moderator = null
)
{
...

   $pendingChanges = $this->finder('XF:UsernameChange')->where([
      'user_id' => $user->user_id,
      'change_state' => 'moderated'
   ]);
   if ($skipChange)
   {
      $pendingChanges->where('change_id', '!=', $skipChange->change_id);
   }

   foreach ($pendingChanges->fetch() AS $pendingChange)
   {
      $pendingChange->whenSaveable(function(\XF\Entity\UsernameChange $pendingChange) use ($user, $moderator)
      {
...
      });
   }
}

This is fetching any moderated changes and then expires duplicates, that makes sense. Except the callback registered in whenSaveable doesn't look to be very called as these are never registered with the calling entity in any way.

I'm fairly sure $pendingChange->whenSaveable should be $user->whenSaveable.

Even then, UsernameChange::_postSave() doesn't modify the $user entity so that isn't going to reliably work either. I'm struggling to see how whenSaveable would be used in this function.

Also shouldn't this function call $user->clearCache('PendingUsernameChange') to ensure a non-cached value is used later?

 

[Mike]

I think you might be misinterpreting what whenSavable does: it simply calls the closure whenever there isn't a write actively running on that entity. If nothing is in the process of being saved, it will be called immediately. If a write is in progess, then it will be saved and automatically called when finished. It's essentially designed to avoid an issue where this could be called in post-save of an entity where writes would fail.

You can test how this works by having a username change pending for a user and then changing their username directly as an admin (ideally to a different username). You'll be able to see a log entry indicating the pending change has been rejected.