Page MenuHomePhabricator

PageArchive::getRevision uses timestamp, suggested replacement uses rev id
Closed, ResolvedPublic

Description

PageArchive::getRevision was deprecated in T194015: Make PageArchive aware of MCR in favor of a new method (the getArchivedRevision, since updated to getArchivedRevisionRecord)

getRevision retrieves the Revision object corresponding to a timestamp. Its replacements retrieve based on revision ids.

How should callers be updated? Specifically, for the method to be hard deprecated, Special:Undelete needs to stop using the method, but I'm unsure how to replace it.

Event Timeline

DannyS712 triaged this task as Medium priority.Apr 11 2020, 7:40 AM
DannyS712 created this task.
DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.
DannyS712 added a subscriber: daniel.

CC @daniel who made the original soft deprecation

How should callers be updated? Specifically, for the method to be hard deprecated, Special:Undelete needs to stop using the method, but I'm unsure how to replace it.

In the HTML form, a "revid" field should be added to accompany the "timestamp" field. The same is true for links generated in getPageLink(), formatRevisionRow(), diffHeader(), etc. Paging would be based on timestamp+revid, not just timestamp - this is really necessary, because the timestamp alone isn't unique, and for a small page size, it's theoretically possible for all rows to have the same timestamp, causing the paging mechanism to loop. This means that PageArchive::getPreviousRevision() should (optionally, for now) take a revId in addition to the timestamp. I hope adding the revision ID to the respective database query won't cause the database to get confused and no longer use the name_title_timestamp index. That index should really be changed to be unique and also include the rev_id, but that's going to far for this patch.

Anyway, it should be possible to provide a revision id in all places (form and links) where we currently have a timestamp. That should be sufficient to change from getRevision() to getArchivedRevisionRecord() in showRevision(). However, there is still the usage in redirectToRevDel(). I didn't investigate where the "ts" values come from, but it seems like we'll still need a method to find the revision ID by timestamp. That method, let's call it findRevisionByTimestamp, could just return a revision ID, or a RevisionRecord. It should make sure that it returns "a" revision with the timestamp, but that the choice may be ambiguous and arbitrary, if there are multiple revisions with that timestamp on the page.

@daniel would you be willing to tackle this?

@daniel would you be willing to tackle this?

Can't commit to it right now, I have other things on my plate. But since this is in the CPT inbox now, we'll triage and prioritize.

Untagging CPT for now, please re-tag if you need code review.

@daniel would you be willing to tackle this?

Can't commit to it right now, I have other things on my plate. But since this is in the CPT inbox now, we'll triage and prioritize.

Untagging CPT for now, please re-tag if you need code review.

So, um, what tag should it have for CPT to work on this? You said leaving it in the CPT would result in it being triaged, but then it was untagged

@daniel would you be willing to tackle this?

Can't commit to it right now, I have other things on my plate. But since this is in the CPT inbox now, we'll triage and prioritize.

Untagging CPT for now, please re-tag if you need code review.

So, um, what tag should it have for CPT to work on this? You said leaving it in the CPT would result in it being triaged, but then it was untagged

{{ping}} - retagging CPT so this is triaged and prioritized

Until a permanent replacement is available, to avoid delaying the Revision deprecating, going to add an @internal method getRevisionRecordByTimestamp for use in core (no deployed uses of PageArchive::getRevision outside of core)

Change 605689 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Add PageArchive::getRevisionRecordByTimestamp

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

Change 605689 merged by jenkins-bot:
[mediawiki/core@master] Add PageArchive::getRevisionRecordByTimestamp

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

DannyS712 closed this task as Resolved.Jun 16 2020, 1:37 AM
DannyS712 claimed this task.
DannyS712 reopened this task as Open.Jun 30 2020, 7:39 AM

Not sure why it hasn't caused issues, but found another use

Change 608470 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Remove another use of PageArchive::getRevision

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608470

hashar added a subscriber: hashar.Jun 30 2020, 7:54 AM

Using [[ https://logstash.wikimedia.org/app/kibana#/dashboard/mediawiki-deprecated

logstash dashboard mediawiki-deprecated ]] and filtering for "PageArchive::getRevision" over 7 days:

Looks like Special:Undelete is the last use case.

Change 608470 merged by jenkins-bot:
[mediawiki/core@master] Remove another use of PageArchive::getRevision

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608470

Change 608472 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@wmf/1.35.0-wmf.38] Remove another use of PageArchive::getRevision

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608472

Change 608473 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@wmf/1.35.0-wmf.39] Remove another use of PageArchive::getRevision

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608473

Change 608473 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.39] Remove another use of PageArchive::getRevision

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608473

Mentioned in SAL (#wikimedia-operations) [2020-06-30T10:30:54Z] <hashar@deploy1001> Synchronized php-1.35.0-wmf.39/includes/specials/SpecialUndelete.php: Remove another use of PageArchive::getRevision - T249982 T254176 (duration: 00m 56s)

After discussion with @DannyS712 , this will be part of 1.35.0-wmf.39.

Change 608472 abandoned by DannyS712:
Remove another use of PageArchive::getRevision

Reason:
Per discussion with hashar, won't be backported, since the fix will go out this week with the 1.35.0-wmf.39 train

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608472

DannyS712 closed this task as Resolved.Jun 30 2020, 10:45 AM
DannyS712 removed a project: Patch-For-Review.