The Community-Tech is currently starting to work on T285514: (Un)delete associated talk page (spike). The first thing we want to do is to add an option to delete the associated talk page when deleting a page. Since the deletion-related code is pretty old and needs to be refactored, we're looking into refactoring it before implementing the new feature (both because it's easier to deal with modern code, and because we need to change the logic behind big deletion checks). Given the expertise of the Platform Engineering team in this area, we'd like to ask for feedback about the refactoring strategy.
Problem: the code for page deletion is currently split into DeleteAction (FormlessAction, just a thin proxy), Article (builds the UI, checks permissions & token, sanity checks, runs some hooks), and WikiPage (some more checks, does the actual deletion at the DB level, logs the deletion). Title::isBigDeletion() is also in the wrong place, since we'd like to sum the revisions count of a page and that of its talk page and compare the result against $wgDeleteRevisionsLimit.
Proposed solution: This is a possible new implementation that I came up with:
- DeleteAction should be a FormAction, responsible for building the UI, putting the form data together, and performing some higher-level checks (e.g. permissions, token, sanity)
- We would then introduce a new service, DeletePage (with a PageCommandFactory factory). This would be a command-style object with the actual deletion logic. It would be responsible for running all the hooks, performing the final checks (including $wgDeleteRevisionsLimit), actually deleting the revisions at the DB level, and logging the deletion.
While refactoring this code, the signatures should be updated to remove legacy parameters and replace wide typehints (e.g. Title, User) with the newer interfaces (e.g. UserIdentity, Authority, PageIdentity, LinkTarget). Hook signatures could remain as they are, for now.
Beyond the general "what do you think about the above plan?" and "has there been any recent work in this area?" questions, I have a few specific questions for the team:
- What interfaces should be used for page parameters? In particular, how to choose between LinkTarget and the many sub-interfaces of PageReference.
- Title::isBigDeletion is currently called from PermissionManager. As mentioned above, we'd want only DeletePage to be aware of "big deletions", but we can't pass a DeletePage to PermissionManager. What would you suggest for that? I can't see an easy way to use DeletePage logic from PermissionManager (e.g. a public static method wouldn't really work), so the only thing I can think of is moving the whole check to DeletePage, but I'm not sure if that change would be going in the right direction.