Page MenuHomePhabricator

The stable to override IndexPager::makeLink is not called breaking custom paging links
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):
Any code that overrided the "@stable to override" method IndexPager::makeLink no longer has this called by at least ReverseChronologicalPager because of the switch to using the PagerNavigationBuilder added in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/803620. This is because the methods that call this code were marked as deprecated but then simply not called.

For an example of what this broke, see the subtask for Special:CheckUser.

What happens?:
The methods, marked as deprecated through the above patch, are not called and thus all the paging links for classes which extend at least ReverseChronologicalPager or AlphabeticPager and which override the stable to override method IndexPager::makeLink are no longer called making the default makeLink code be always used

What should have happened instead?:
The code using the previous method should still be supported. Not doing this violates the https://www.mediawiki.org/wiki/Stable_interface_policy because the method was marked as stable to override and providing a route to keep existing functionality (which has not been done here) seems possible to me.

Software version (skip for WMF-hosted wikis like Wikipedia):
Master branch of core

Other information (browser name/version, screenshots, etc.):
See the subtask for CheckUser for screenshots of the source for what was before and after the last train.

Even if all uses were updated before the 1.39 release the PagerNavigationBuilder::makeLink is a private method that means it prevents custom paging links from being used.

Event Timeline

Dreamy_Jazz renamed this task from IndexPager's getPagingLinks is now unused following Ic75bd597b210e14612ca3aebb531b659897e8294 to IndexPager's getPagingLinks, getLimitLinks and makeLink are not called even though they are only deprecated.Sep 10 2022, 4:14 PM

This is a high priority task as it breaks code that overrides "stable to override" methods such as makeLink without a proper deprecation process. This is likely to affect more than just CheckUser.

Change 830985 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/core@master] Revert "Introduce PagerNavigationBuilder for making pagination links"

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

I've added this as a release blocker as this would affect any code that overrides said stable to override method and a blocker for wmf.28 because it broke paging in Special:CheckUser in that train.

Dreamy_Jazz raised the priority of this task from High to Unbreak Now!.Sep 10 2022, 4:49 PM
Dreamy_Jazz updated the task description. (Show Details)

Making UBN as I've added it as a blocker for at least the release.

Dreamy_Jazz renamed this task from IndexPager's getPagingLinks, getLimitLinks and makeLink are not called even though they are only deprecated to The stable to override soft deprecated IndexPager::makeLink is not called.Sep 10 2022, 5:14 PM
Dreamy_Jazz renamed this task from The stable to override soft deprecated IndexPager::makeLink is not called to The stable to override IndexPager::makeLink is not called.
Dreamy_Jazz renamed this task from The stable to override IndexPager::makeLink is not called to The stable to override IndexPager::makeLink is not called breaking custom paging links.
Dreamy_Jazz updated the task description. (Show Details)

Change 831196 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Restore compatibility with overrides for IndexPager::makeLink()

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

I'm sorry about this, I overlooked it when working on my changes.

The patch above works in my testing, but I'm not too familiar with CheckUser, so please check whether it works with everything.

It also provides a non-deprecated way to achieve the same – override IndexPager::getNavigationBuilder() to return a subclass of PagerNavigationBuilder, and override PagerNavigationBuilder::makeLink() as desired. I planned for this from the start, but I forgot to make that method actually overridable :( Please check whether this works for you as well.

Thanks for the quick fix. I will test this now.

Applying this patch Special:CheckUser makes the links work as expected again. The default is no longer used which means calls to makeLink are now working again. Thanks.

This patch will need to be backported to REL1_39 if it's merged in.

Change 830985 abandoned by Dreamy Jazz:

[mediawiki/core@master] Revert "Introduce PagerNavigationBuilder for making pagination links"

Reason:

Should be fine to go with the patch that addresses the issue of not calling IndexPager::makeLink

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

Change 831196 merged by jenkins-bot:

[mediawiki/core@master] Restore compatibility with overrides for IndexPager::makeLink()

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

Change 831214 had a related patch set uploaded (by Jforrester; author: Bartosz Dziewoński):

[mediawiki/core@REL1_39] Restore compatibility with overrides for IndexPager::makeLink()

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

Change 831215 had a related patch set uploaded (by Jforrester; author: Bartosz Dziewoński):

[mediawiki/core@wmf/1.39.0-wmf.28] Restore compatibility with overrides for IndexPager::makeLink()

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

Change 831214 merged by Jforrester:

[mediawiki/core@REL1_39] Restore compatibility with overrides for IndexPager::makeLink()

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

Dreamy_Jazz assigned this task to matmarex.

Has been cherry-picked to MediaWiki 1.39 release and the train is now on 1.40-wmf.1 so putting into 1.39.0-wmf.28 would not affect any wikis. Testing on enwiki shows this is now working again for CheckUser. A code search found that this would have affected CentralNotice, but this should be addressed like how CheckUser's issue was addressed.

Change 831215 abandoned by Jforrester:

[mediawiki/core@wmf/1.39.0-wmf.28] Restore compatibility with overrides for IndexPager::makeLink()

Reason:

Yup.

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