Page MenuHomePhabricator

Special:OldReviewedPages always uses diffonly=0, ignoring user preferences
Closed, DeclinedPublic

Description

The review links on this specialpage seem to ignore the diffonly setting. Such is unexpected behavior for people.


Version: unspecified
Severity: enhancement

Details

Reference
bz24315

Event Timeline

bzimport raised the priority of this task from to Lowest.Nov 21 2014, 11:04 PM
bzimport set Reference to bz24315.

It was meant to help people see the whole page (template/files can change), though probably isn't needed so much on enwiki. (bug 14828)

To be specific, it is there *to* override it for those diffs.

I'm adding the original requestor of bug 14828 to the cc line here. I think we should revisit this one.

Aaron's right. The reasoning was and is (at least for me):

  • If you are on your turf, i. e., browsing an article history or your watchlist, the user configuration should prevail.
  • If you are looking at changes in articles you have never seen before, you can't assess the effect of the change with only one line of context, so provide full context.
  • If there are changes in included templates and images, provide full context as well.

It would seem to me that if someone puts "diff only" in their preferences, they really want diff only.

There are currently performance implications with using this with diffonly=0. We're working on fixing those, but they won't entirely go away. If someone has indicated they're not likely to view the preview below the diff, I don't think we should force it on them.

Eh, no. I have switched on "diff only" in every wiki I have ever used, and yet I requested the original change here. And I even backed that with arguments, not with "wants".

And this change has been active on dewp for almost two years, which seems to suggest that the reasoning wasn't *that* bad.

So it would be really nice if the almighty "performance implications" could be explained a bit more detailed: How can something that worked on dewp for almost a million articles for almost two years melt the servers on enwp with less than 2000 articles (and, at this moment, three (3) articles on Special:OldReviewedPages)?

(In reply to comment #5)

There are currently performance implications with using this with diffonly=0.
We're working on fixing those, but they won't entirely go away.

Once we get that sync'd, I consider it a non-issue. The wider question of caching parser output of older revs like we discussed the other day is way beyond the scope of this bug.

(In reply to comment #6)

Eh, no. I have switched on "diff only" in every wiki I have ever used, and yet
I requested the original change here. And I even backed that with arguments,
not with "wants".

I agree with what you're saying. I'd actually suggest we WONTFIX this in favor of keeping the behavior as requested in bug 14828

And this change has been active on dewp for almost two years, which seems to
suggest that the reasoning wasn't *that* bad.

That's a pretty convincing argument as well.

So it would be really nice if the almighty "performance implications" could be
explained a bit more detailed: How can something that worked on dewp for almost
a million articles for almost two years melt the servers on enwp with less than
2000 articles (and, at this moment, three (3) articles on
Special:OldReviewedPages)?

It didn't "melt the servers" :)

As explained on bug 24124, when viewing a diff, the preview of content under the diff was being parsed on every diff view and not using the parser cache. When viewing a diff to the current rev, which is a very common scenario, I changed it in r69191 to use the parser cache.

It's only a fix for a single scenario (diffing to cur), but it's a fairly common scenario in the enwp setup for FlaggedRevs so it should provide a noticeable difference. And it'll be generally useful whenever someone diffs to cur.

Resolving as "wontfix". It may be better for us to look into removing the diffonly pref altogether than spending a lot of effort making it work. Regardless, since there's not a strong consensus to change this, we'll leave it as is unless the situation changes.