Page MenuHomePhabricator

Special:Block should not play the role of a central blocking service/utility
Closed, ResolvedPublic

Description

Overview

The SpecialBlock class has outgrown its purpose as a special page and become both a blocking service and utility class for other blocking features. Classes that depend on SpecialBlock include ApiBlock, ApiUnblock, SpecialUnblock, and Language, as well as AbuseFilter, GlobalBlocking and CheckUser extensions.

The two main ways in which Special:Block goes beyond its role as a special page are:

  1. Acts as a service for making blocks, called by the API. Instead, we should have a service (or system of services) for dealing with blocks, which SpecialBlock and the other classes/features use. (SpecialUnblock plays a similar role for unblocking.)
  2. Acts as a utility class for holding methods related to input handling. These stem largely from widget deficiencies. If we can't fix these right away, we could at least make them more obvious.

Details

Performing blocks/unblocks

SpecialBlock::processForm and SpecialUnblock::processUnblock are each used by their respective APIs as to actually perform blocks/unblocks. However, these methods do more than that: they also process input, do permissions checks, contain logic about the user's intent, handle user suppression (T213981) and log the action.

As a result, the two methods are a mixture of several different functionalities, and also contain logic that should be specific to either the API (e.g. log tagging, 'Reblock' field) or the special page (e.g. 'Confirm' field).

Additionally, SpecialUnblock::processUnblock depends on the SpecialBlock class for permission checks and target parsing.

The common functionality for placing new blocks, updating existing blocks and removing blocks should be moved to a service that can be used from the APIs and special pages, and any other extension blocking features (e.g. AbuseFilter, GlobalBlocking, CheckUser). This could be one service, or separate services for blocking and unblocking (with both depending on with another service for things they have in common, like permission checks). What's best might depend on how much commonality there is. (See also T189073, T250020.)

Methods for permission checks are:

  • SpecialBlock::checkUnblockSelf - also used by ApiBlock, ApiUnblock, SpecialUnblock
  • SpecialBlock::canBlockEmail - also used by ApiBlock, SpecialCheckUser

Unpicking SpecialBlock::processForm is intricate work, so we should ensure good test coverage first.

Utilities/input handling

SpecialBlock::getTargetAndType

  • Used by SpecialBlock and ApiBlock as wrapper for DatabaseBlock::getTargetAndType. In those cases it would be better to call that method directly.
  • Used by SpecialBlock::setParameter and SpecialUnblock::execute to prioritize request parameters. There doesn't seem to be a reason why the request parameters should stay in sync between the two special pages, so this method could be simplified and reimplemented on each class.

SpecialBlock::validateTarget

  • This is used by ApiBlock and SpecialBlock, and duplicates a lot from HTMLUserTextField (T177329). Ideally the logic should move to a common place that the API, special page and form field can access. (We'd need to ensure that the appropriate errors are returned, as some messages have been customized for blocking.)

SpecialBlock::getSuggestedDurations

  • Also used by AbuseFilter and Language::translateBlockExpiry. This allows wikis to customize default durations offered by the expiry widget for blocks. If the expiry widget has the same durations for any block feature, should we instead have a blockExpiryWidget with these defaults?

SpecialBlock::parseExpiryInput

  • Also used by AbuseFilter, GlobalBlocking, CheckUser. This is essentially a utility method that parses an expiry time in English (T190449) to a timestamp. It would be needed wherever an expiry field is used (though it's currently only used on block features). It should therefore be somewhere more general than a block service.

Proposal for tasks

The work could be split into the following tasks:

  • Ensure SpecialBlock::processForm, SpecialUnblock::processUnblock (and the methods that call them) have good test coverage (T250989)
  • Refactor SpecialBlock::processForm, SpecialUnblock::processUnblock and permissions checks into a block service (T189073, T250020)
  • Don't call SpecialBlock::getTargetAndType unnecessarily (i.e. whenever it is called with only one argument) (T250940)
  • Replace SpecialBlock::getTargetAndType with separate methods on SpecialBlock and SpecialUnblock (T250940)
  • Move logic from SpecialBlock::validateTarget to common place accessible by SpecialBlock, ApiBlock and HTMLUserTextField (T177329)
  • Implement HTMLBlockExpiryField and move SpecialBlock::getSuggestedDurations there
  • Move SpecialBlock::parseExpiryInput somewhere more general (T248196)

Related Objects

Event Timeline

Change 628429 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Deprecate SpecialBlock::checkUnblockSelf

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

Change 628432 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Move SpecialBlock::canBlockEmail to BlockPermissionChecker

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

I believe this one is mostly done