Page MenuHomePhabricator

(Un)delete associated talk page (spike)
Closed, ResolvedPublic2 Estimated Story PointsSpike

Description

Allow admins and patrollers the ability to (Un)delete associated talk page.

Wish page

Project page

Event Timeline

Outcome of the investigation

Detailed goals

These are the exact goals that I'd set for this project:

  1. When deleting non-talk articles (action=delete), there should be a checkbox to allow deleting the associated talk page, if it exists
  2. There should be a similar option in the action=delete API module
  3. When undeleting non-talk articles (Special:Undelete), there should be a checkbox to allow undeleting the associated talk page, if it was deleted
  4. There should be a similar option in the action=undelete API module

Considerations about community input

The project goals were set quite clearly on the wish page, and from the user perspective these are all small changes. As such, I don't think any particular community input is necessary. Of course, this might change at any time, e.g. if we find out that implementing the feature exactly how it was requested is not possible for some reason. And it also doesn't mean that we don't want any input from the community; it just means that we can likely move this forward without being blocked on such input.

Technical feasibility

Both parts (deletion and undeletion) should be doable, as partly demonstrated by this patch. Core already has the base functionality that we need (e.g. to retrieve the associated talk page), so we only need to use it where necessary. If we could ignore code quality and get away with an ugly and naive approach, this task would be trivial.

Security/performance considerations

No security-related concerns; the additions to the UI are small, and I can't see any potential exploitation of the new feature.

Performance-wise, the main concern is what would happen with talk pages having a lot of revisions. The deletion code restricts so called "big deletions" (see Title::isBigDeletion) to highly privileged users, which for WMF production means stewards (and botadmins on ckbwiki). We must obviously enforce this restriction for talk pages. However, it might still be a problem if both the main page and the talk page have a large number of revision. For instance, if we take the revision count limit of 5000 used in WMF production, we could have an edge case where the main page and the talk page have 4999 revisions each, and a deletion would effectively result in 9998 revisions being deleted. It should be properly evaluated whether this is a sustainable/acceptable cost. If not, we could consider using the 5000 limit for the total number of revisions being deleted, although the current code doesn't allow that.

Foreseeable difficulties

The foremost difficulty with this wish is that the code that we need to touch is mostly legacy code, written years ago and with a lot of tech debt. This means that it's harder to reason about that code, it's harder to write clean code, and it's also harder to make sure that nothing will break. Refactoring such a massive amount of legacy code is beyond the scope of this task. The best we can do is to avoid adding more hacks and tech debt, thus leaving the tech debt essentially unchanged.

Overview of the relevant code

There are two sections of code that we need to change, one for deletion and one for undeletion, and these are described separately.

Deletion
UI

The deletion interface is not a special page, but rather a formless action, DeleteAction. This is effectively a thin proxy that delegates everything to Article::delete. Article is where we check permissions, the form fields, the CSRF token, where we generate the HTML form etc., calling WikiPage::doDeleteArticleReal to perform the actual deletion. As you might have guessed, the Article is plagued by tech debt.

API

The relevant module is ApiDelete. We check form data and permissions there, and then delegate to WikiPage::doDeleteArticleReal. This code is much cleaner than its counterpart in Article.

Common

As described above, the UI and API code paths cross in WikiPage::doDeleteArticleReal. This method runs a hook and then delegates to WikiPage::doDeleteArticleBatched. There we start an atomic DB section and actually delete a page at DB-level. Nothing in WikiPage checks the user's permissions.

Undeletion
UI

Undeletion is implemented via a proper special page, SpecialUndelete. The class is responsible for building the form, checking permissions etc., and delegates the actual undeletion to PageArchive::undeleteAsUser.

API

The API module is ApiUndelete. It checks permissions etc. and then also delegates to PageArchive::undeleteAsUser.

Common

PageArchive is where the actual undeletion takes place, more precisely in PageArchive::undeleteRevisions. This method is similar to WikiPage::doDeleteArticleBatched, although it's not DB-only: it first scans the DB for rows to restore, and then restores those revisions one by one, which implies more queries and, I think, worse performance. T257298 proposes to add batching to undeletion, but that's not being worked on.

Technical implementation details

Deletion

Permission checks are currently duplicated in Article and ApiMove. Since we also need permission checks for the associated talk page, we're going to add some logic in those two places. This is unfortunate, but the proper solution would be to have an intermediate PageDelete service (à la PageMove). I believe that duplicating the logic (with a few TODOs) is better than adding another public method to Article. Then, in both places we'd have 2 calls to doDeleteArticleReal. In ApiDelete, we could just call delete() twice. In Article, I think we cannot just call doDelete() twice, because that method does a few extra things, so either we add a call to doDeleteArticleReal there, or extract another private method if there's some common code to reuse.

Note, if we also want to change the logic behind isBigDeletion, things become more complicated, since that logic lives in Title, but we'd want it to work for two Titles at the same time. If changing this logic is necessary for performance, we might well just go ahead and extract a page deletion service, since I can't think of an easy and non-super-hacky way to make that logic work for two titles at the same time.

Another thing to pay attention to is what happens if the base page deletion succeeds, but the talk page deletion doesn't. Showing an error (like for page moves) might be the best options, or at least the easy one.

Undeletion

Since we also lack a PageUndelete service, the permission check logic is also going to be duplicated. All in all, we're going to need the same kind of changes that we need for Article/ApiDelete, but this code is a bit cleaner and should be easier to refactor.

High-level implementation plan

I'd propose to have two parts, doing the deletion first and the undeletion later (or the other way around). This is motivated by the two features having essentially no common code, avoiding big patches, and also having more time for QA between the two parts.

Conclusions

It's possible to implement these features, but the tech debt around the relevant code is going to make things harder. Things should be carefully written and reviewed to ensure that nothing breaks. Ideally we'd want to avoid massive refactorings and try not to make the code worse while we add new logic. The main question I have is the one about performance. If we need to avoid potentially large deletes in a single request, refactoring this code will be necessary (kind of). This should be discussed before starting the actual work.

A few additional notes:

  • We shouldn't need a config variable for this. A temporary feature flag at most, maybe not even that
  • For undeletion, we should only show the checkbox if the user is restoring the whole page (and something similar for the API module)
Restricted Application changed the subtype of this task from "Task" to "Spike". · View Herald TranscriptAug 10 2021, 8:15 PM
ldelench_wmf set the point value for this task to 2.Aug 10 2021, 8:15 PM
DannyS712 renamed this task from (Un)delete associated talk page to (Un)delete associated talk page (spike).Aug 11 2021, 9:08 AM