Page MenuHomePhabricator

Refactor Block to AbstractBlock, DatabaseBlock and SystemBlock
Closed, ResolvedPublic3 Story Points

Description

In order to complete T206163, the Block class needs to be refactored so that blocks made from multiple blocks can be separated out from single blocks. In doing this, we will create an AbstractBlock parent class which can be extended by a class for single blocks and a new class for multiple blocks (CompositeBlock). The CompositeBlock class is convenient, because it allows the rest of the codebase (except the BlockManager) to be unaware of overlapping blocks.

While we are doing this, it makes sense also to separate out system blocks, which are temporary blocks usually created against IP addresses listed in global configs. These are single blocks but are never saved to the database.

In summary, Block will be refactored into the following classes:
AbstractBlock - abstract parent class
DatabaseBlock - blocks that are saved to the database
SystemBlock - temporary blocks

In addition there will be:
CompositeBlock - blocks made from several blocks

Block remains as a deprecated alias for DatabaseBlock, the most common type of block.

Breaking changes

This work breaks any cases where typehints for Block could be expecting a SystemBlock or a DatabaseBlock. These need to be updated to AbstractBlock. (Such cases are unusual - in most cases only a DatabaseBlock is expected.)

Where a Block typehint expects only a DatabaseBlock, it should still be updated since Block is deprecated, but will not break for now.

Naming

There have been some discussions about an alternative naming scheme, in which the parent class is named Block and the class for database blocks is called DatabaseBlock. This avoids the break mentioned above, but would break calls to static methods such as Block::newFrom* and calls to the constructor new Block(). Given the prevalence of these across extensions, this would be a much less convenient change.

Plan

  • Introduce an empty AbstractBlock class, and have Block extend it. This is just to avoid the breaking change mentioned above and will be quickly followed-up with the refactoring patch.
  • Update any typehints in extensions that use Block (but may expect a SystemBlock or CompositeBlock), based on: https://codesearch.wmflabs.org/deployed/?q=(%3F%3A%5CsBlock%5Cs%7Cinstanceof%5CsBlock)&i=nope&files=&repos= (NB most of these examples are in fact expecting a database Block, so do not need to be updated.)
  • Split Block into AbstractBlock, Block and SystemBlock
  • Make Block a deprecated alias of DatabaseBlock, which is more descriptive
  • Communicate and verify documentation of the soft deprecation

Event Timeline

Tchanders created this task.May 7 2019, 4:07 PM
Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptMay 7 2019, 4:07 PM

Change 504610 had a related patch set uploaded (by Mooeypoo; owner: Tchanders):
[mediawiki/core@master] Add AbstractBlock parent class for Block

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

Change 497667 had a related patch set uploaded (by Mooeypoo; owner: Tchanders):
[mediawiki/core@master] Separate Block into AbstractBlock, Block and SystemBlock

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

Change 497668 had a related patch set uploaded (by Mooeypoo; owner: Tchanders):
[mediawiki/core@master] WIP Add MultipleBlock class for enforcing multiple blocks

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

Tchanders updated the task description. (Show Details)May 7 2019, 5:02 PM

Change 504610 merged by jenkins-bot:
[mediawiki/core@master] Add AbstractBlock parent class for Block

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

Change 508623 had a related patch set uploaded (by Dmaza; owner: Dmaza):
[mediawiki/extensions/TorBlock@master] Update type hint to parent class AbstractBlock

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

Change 508623 merged by jenkins-bot:
[mediawiki/extensions/TorBlock@master] Update type hint to parent class AbstractBlock

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

Mooeypoo updated the task description. (Show Details)May 7 2019, 9:28 PM
Mooeypoo updated the task description. (Show Details)
Niharika set the point value for this task to 3.May 7 2019, 10:24 PM

Change 497667 merged by jenkins-bot:
[mediawiki/core@master] Separate Block into AbstractBlock, Block and SystemBlock

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

Change 509886 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Rename Block to \MediaWiki\Block\DatabaseBlock

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

Change 509897 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Update release notes following the refactor of Block

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

Change 509897 merged by jenkins-bot:
[mediawiki/core@master] Update release notes following the refactor of Block

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

dom_walden moved this task from QA/Testing to Done on the Anti-Harassment (Yōd - י) board.EditedMay 15 2019, 11:33 AM

Are Blocks stored correctly in DB?

Blocks and autoblocks appear in Special:BlockList with the correct values (incl.
pages, namespaces).

I also briefly tested updating and deleting blocks, in case pages/namespaces are not added/removed properly from the database.

Is the correct block returned for a user/ip?

Tested several combinations of conditions where more than one block might apply
to a user, incl.:

  • Username block + autoblock and IP Range block, with $wgCookieSetOnIpBlock and $wgCookieSetOnAutoblock enabled. Logged in and logged out, with and without Tor.

Range block has higher precedence than IP Autoblock. Tor blocks have a higher precedence than anything else, and blocks account create.

  • Different IP and IP Range blocks. Doing actions via the API with different IPs set in XFF headers.

Blocks which match the original IP have precedence over blocks which match IPs in XFF headers.

Testing was done on https://en.wikipedia.beta.wmflabs.org.
Where I was unsure if the behaviour was correct I compared with https://test.wikipedia.org.

Tested individual methods in the block classes

This change introduced a few new classes and, although much of this is covered by unit testing, I wanted to make sure I was testing each method in each class at least once, just in case they are incorrectly implemented.

On my local machine I setup my server to record code coverage.

I tried to test each type of block with a range of actions to get the most coverage. Checked the code coverage to see if I was calling each method (including methods inherited from parent classes, although this is harder to tell).

The report is tarred up here

. I touched most of the methods at least once, I missed some of the stuff related to cookies (but I have tested these elsewhere).

Briefly tested User block, autoblock, IP (anon), IP range (logged in and anon). Came across this preexisting bug: https://phabricator.wikimedia.org/T15611

I then tested system blocks:

  • $wgProxyList:

IPs blocked in $wgProxyList can still create accounts. This was supposedly fixed here https://phabricator.wikimedia.org/T36385 but did not work for me on master or 1.32.1 (so predates this work). I will re-raise this bug somehow.

Logged in user not blocked from email or account create if Proxy blocked, but blocked from everything else, incl. password reset.

  • $wgDnsBlacklistUrls:

Anon. users blocked from everything (incl. account create).

DNSBL block does not affect logged in users (but are still blocked from account create).

This is the same on MW 1.32.1.

...and some of the more obscure things:

  • $wgBlockDisablesLogin
  • $wgBlockAllowsUTEdit
  • hideuser
  • global blocks

Hideuser blocks don't appear in block list, user's contribs crossed out, as well as revisions in article history.

Global blocks blocks all actions apart from email. Tested anon. and non-anon. global blocks.

What I did not test (e.g. extensions)

Apart from the Tor and GlobalBlocks extensions and incidentally doing a bit of testing of VisualEditor (finding T209599#5168073), I have not tested any other extensions.

I think the testing above is enough to suggest that nothing has changed as far as the external interface of blocks is concerned. But I might be wrong.

dbarratt closed this task as Resolved.May 22 2019, 6:06 PM
Tchanders reopened this task as Open.May 24 2019, 1:23 PM

Reopening because the checklist isn't complete

Tchanders updated the task description. (Show Details)May 24 2019, 1:24 PM
Tchanders moved this task from Done to Review on the Anti-Harassment (Yōd - י) board.
Mooeypoo added a subscriber: Mooeypoo.

There's not a lot of specifics to test here, but we should just verify that things didn't break with this on beta.

We can proceed writing the communication for wikitech-l, summarizing the work.

Change 509886 merged by jenkins-bot:
[mediawiki/core@master] Rename Block to MediaWiki\Block\DatabaseBlock

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

Tchanders updated the task description. (Show Details)May 29 2019, 8:38 PM
Tchanders updated the task description. (Show Details)

[mediawiki/core@master] Rename Block to MediaWiki\Block\DatabaseBlock
https://gerrit.wikimedia.org/r/509886

See T224704: CI build failing for some ext repos due to PhanUndeclaredClassMethod "undeclared class \Block", which seems likely caused by this change.

Tchanders updated the task description. (Show Details)Jun 4 2019, 7:36 PM
Niharika triaged this task as Normal priority.Jun 4 2019, 9:43 PM

Smoke testing on https://en.wikipedia.beta.wmflabs.org, including:

  • Blocking and unblocking via web and API.
  • Editing via API while partial blocked.
  • Moving title via API.
  • Looking at blocked user's (deleted) contrib.
  • Tested file upload while partially blocked from File.

Also tested a couple of extensions effected by T224704:

  • PageTriage
  • GlobalBlocking
Niharika closed this task as Resolved.Jun 5 2019, 10:15 PM
Tchanders updated the task description. (Show Details)Jun 20 2019, 2:56 PM