Page MenuHomePhabricator

Broken prev diffs when apply tag filter to history
Closed, ResolvedPublic

Description

Looking at the history of a user's talk and entering "discretionary sanctions alert" for the Tag filter shows an extract from the history.

Clicking the "prev" diff in any line should show a difference of the alert being added. Currently, however, the diff is meaningless because it gives the difference between successive entries in the filtered history. In this example, the first two entries are dated 5 August 2018 and 6 July 2018 because an alert was added on each of those dates. However, the prev diff on the first line gives the difference from 6 July 2018 to 5 August 2018 which is no use.

I mentioned this at en:WP:VPT. I think that the prev diff worked in the past.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Suspect that this is caused by https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/c9c1ebd330fb402e66bbba2b1c5dc7562a07eb27/includes/actions/pagers/HistoryPager.php#565
As far as I can see, this has always been the case, since a separate PageHistory::lastLink function was created in https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/22c813ad9784bceca4220a558910fd5de00dbd4b (2005). The code before that is hard to understand
@brion is there any specific reason not to use prev instead of providing the (usually prior, but not when filtering) oldid manually?

Change 600517 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] HistoryPager: 'prev' link should ignore filters

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

Aklapper added a subscriber: DannyS712.

Removing task assignee due to inactivity as this open task has been assigned for more than two years. See the email sent to the task assignee on August 22nd, 2022.
Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome!
If this task has been resolved in the meantime, or should not be worked on ("declined"), please update its task status via "Add Action… 🡒 Change Status".
Also see https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator. Thanks!

A similar issue is that the last history row does not have a "prev" link. IMO this is pretty annoying even in normal view, but you can understand the thought behind it; but it's incorrect in a filtered view.

is there any specific reason not to use prev instead of providing the (usually prior, but not when filtering) oldid manually?

Pretty sure we did not have filtering when the code for prev links was originally written.

Change 600517 merged by jenkins-bot:

[mediawiki/core@master] HistoryPager: 'prev' link should ignore filters

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

Thanks for the patch and review. Contributions etc. use diff=prev&oldid=..., but the patch seems to make history use diff=...&oldid=prev. Feel like it should be consistent with the former (so links to the same diff appear as visited, among other benefits).

Also, I have a script that assumes oldid in prev links on history includes revision IDs. I imagine there are other things that can break as a result of this change, so I think it should be announced in User-notice.

Thanks for the patch and review. Contributions etc. use diff=prev&oldid=..., but the patch seems to make history use diff=...&oldid=prev. Feel like it should be consistent with the former (so links to the same diff appear as visited, among other benefits).

Yeah that's weird, but it wasn't introduced by this patch. Not sure if there's any reason for it.

Also, I have a script that assumes oldid in prev links on history includes revision IDs. I imagine there are other things that can break as a result of this change, so I think it should be announced in User-notice.

Hopefully that script didn't do anything important as the oldid has been pointing to the wrong revision until now :)

Example Tech News text: When page history was filtered by a tag, the "prev" link incorrectly showed the difference between the given revision and the previous revision with the same tag, instead of the previous revision overall. This has been fixed.

In T243569#8518302, @Tgr wrote:

Thanks for the patch and review. Contributions etc. use diff=prev&oldid=..., but the patch seems to make history use diff=...&oldid=prev. Feel like it should be consistent with the former (so links to the same diff appear as visited, among other benefits).

Yeah that's weird, but it wasn't introduced by this patch. Not sure if there's any reason for it.

What makes you say that?

Doesn't

'diff' => 'prev',
'oldid' => $prevRev->getId()

instead of

'diff' => $prevRev->getId(),
'oldid' => 'prev' // T243569

simply make it consistent with links in contributions, "← Older edit", etc.?

Example Tech News text: When page history was filtered by a tag, the "prev" link incorrectly showed the difference between the given revision and the previous revision with the same tag, instead of the previous revision overall. This has been fixed.

This is inaccurate and misses the point. The point is that the patch makes it always use prev instead of a revision ID, even when not filtered by a tag. That's what possibly breaks scripts.

In my OP at the top of this page I gave a link in "this example". I just tried that link again and it is still showing a very unhelpful diff.

The merged patch hasn't been deployed yet.

Change 879168 had a related patch set uploaded (by Nardog; author: Nardog):

[mediawiki/core@master] HistoryPager: Make 'prev' URL consistent with other diff links

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

It seems somewhat random which style is used where, although 'diff' => 'prev' is the more common one.

We could unify them, maybe it would improve caching a bit - not sure how edge caches handle diff pages (I would have thought Varnish has some custom rules but grepping for oldid or diff doesn't yield anything). It would somewhat improve UA visited link indicators, although can be other differences (e.g. RC/watchlist puts the curid in there).

Example Tech News text: When page history was filtered by a tag, the "prev" link incorrectly showed the difference between the given revision and the previous revision with the same tag, instead of the previous revision overall. This has been fixed.

This is inaccurate and misses the point. The point is that the patch makes it always use prev instead of a revision ID, even when not filtered by a tag. That's what possibly breaks scripts.

Hi, for Tech News, please could someone clarify what a clear (and ideally simple and easy to translate) description of the problem was? and also confirm that it is now fixed?
E.g. If I try to simplify the previously suggested draft, it might be something like

"Last week, when a page history had been filtered by a tag, the "<tvar name="prev">{{int:last}}</tvar>" link could incorrectly show more revisions. This has now been fixed. [1]"

Please amend that as needed, or add directly. Thanks.

Something like

⚒️ The oldid parameter in "prev" links on page history is now set to prev rather than the actual ID of the preceding revision. This is to fix such links pointing to incorrect diffs when history was filtered by a tag. Some user scripts may break as a result of this change.

Or if my patch is merged

⚒️ The URLs in "prev" links on page history now contain diff=prev&oldid=[revision ID] in place of diff=[revision ID]&oldid=[revision ID]. This is to fix such links pointing to incorrect diffs when history was filtered by a tag. Some user scripts may break as a result of this change.

Moved to Not ready to announce because we would have to follow it up if my patch is merged after an announcement. @Tgr I urge you to +2 before next deployment because we would have to announce it twice, and maintainers of affected scripts would have to fix it twice.

UPDATE: The patch has been merged.

Change 879168 merged by jenkins-bot:

[mediawiki/core@master] HistoryPager: Make 'prev' URL consistent with other diff links

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

Thanks @Nardog ! I've added it (with minor tweaks) to https://meta.wikimedia.org/wiki/Tech/News/2023/03 - It will be frozen for translations in ~2 hours, if you/anyone needs to make any edits before then, please do.

For anyone whose scripts this breaks: instead of parsing the diff/oldid parameters from the URL, you can get the calculated values (revision IDs for the revisions actually shown in the comparison) using mw.config.get( 'wgDiffOldId' ) and mw.config.get( 'wgDiffNewId' ).

@matmarex In my case I relied on extracting the ID of the preceding revision from oldid on page history, not on the diff page itself. That's no longer possible for the earliest revision on the page or when the list is filtered by a tag.

Nardog assigned this task to DannyS712.