Page MenuHomePhabricator

Removal of RangeChronologicalPager::rangeConds breaks Special:CheckUser's period selection and causes CheckUser pipeline failures
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • Load Special:CheckUser
  • Choose a period of something other than 'all'
  • Run the check

What happens?:
The results returned are from all time

What should have happened instead?:
The results should have been time limited

Software version (skip for WMF-hosted wikis like Wikipedia):
CheckUser and core on the master branch

Other information (browser name/version, screenshots, etc.):
Caused by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/863446. The phan check to flag this as an issue did not come until later, and obviously there wasn't a check on whether this would affect those who override the stable to override method.

Example (note the from the last week only and how it shows data from November):

image.png (805×1 px, 97 KB)

Event Timeline

While this does not lead to a server error, it makes choosing the period for a check do nothing which breaks existing functionality. Also unsure how this removal will effect other parts of the code that may interface with it.

Marking as high as this breaks existing functionality in CheckUser and causes phan test failures on code that would be deployed if the train continued.

Not sure how to address this at the moment. Will require either a revert to the patch that changed how the stable to override method worked or will need a change to CheckUser. Not sure how the change to CheckUser would work.

JJMC89 raised the priority of this task from High to Unbreak Now!.Dec 13 2022, 2:47 AM
JJMC89 subscribed.

As a train blocker

I'll have a look in the morning if this has not already been fixed by someone else. I suspect the fix would be to modify CheckUser to use $this->startOffset and $this->endOffset, but not sure yet as I'm too tired to work this one out.

Change 866821 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] RangeChronologicalPager: Restore the compatibility with derived classes

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

Change 866821 merged by jenkins-bot:

[mediawiki/core@master] RangeChronologicalPager: Restore the compatibility with derived classes

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

Change 867237 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@wmf/1.40.0-wmf.14] RangeChronologicalPager: Restore the compatibility with derived classes

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

Thanks for the work on this @Func. Your patch looks to have fixed the problem. I'll create a separate task to remove the usage of rangeConds and replace it with the new method.

(Actually best wait till the backport is merged to close).

Change 867237 merged by jenkins-bot:

[mediawiki/core@wmf/1.40.0-wmf.14] RangeChronologicalPager: Restore the compatibility with derived classes

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

Mentioned in SAL (#wikimedia-operations) [2022-12-13T15:18:53Z] <derick@deploy1002> Started scap: Backport for [[gerrit:867237|RangeChronologicalPager: Restore the compatibility with derived classes (T228431 T325034)]]

Mentioned in SAL (#wikimedia-operations) [2022-12-13T15:20:38Z] <derick@deploy1002> derick and func: Backport for [[gerrit:867237|RangeChronologicalPager: Restore the compatibility with derived classes (T228431 T325034)]] synced to the testservers: mwdebug1001.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug1002.eqiad.wmnet, mwdebug2001.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2022-12-13T15:27:53Z] <derick@deploy1002> Finished scap: Backport for [[gerrit:867237|RangeChronologicalPager: Restore the compatibility with derived classes (T228431 T325034)]] (duration: 08m 59s)

Thank you both for spotting and fixing the issue, and sorry for breaking CheckUser again. I'll try to be more careful when reviewing and writing pager-related patches in the future.