Page MenuHomePhabricator

Create BlockStore service
Closed, DuplicatePublic

Description

The purpose of this task is to allow application logic to obtain a Block object based on a UserIdentity object (or some other kind of block target) without the need to have a User object. BlockStore is to replace the usage of User::getBlock (and related methods) in PermissionManager (see T208768) and other pplaication logic.

Note that eventually, Block should become a value object (or be replaced by a value object) that has no logic for modifying the database, and no knowledge of the underlying schema. All logic related to the DB schema should be moved to BlockStore. That however is beyond the scope of this ticket. This ticket is a partial refactoring, leaving Block and BlockStore tightly coupled to each other until more functionality is factored out of Block. This fact should be noted as a caveat in the class level documentation of the new class.

For now, the following methods should be moved into the new BlockStore service:

Methods expected to move into the new class:

  • User::getBlockedStatus (private)
  • User::getBlock()
    • User::isBlocked() should probably be deprecated in favor of BLockStore::getBlock(). Same for blockedBy(), blockedFor(), getBLockId(), etc.
    • Note that isBlockedFrom() is currently being moved to PermissionManager, not BlockStore. Maybe that should be re-considered, or the method could be present in both interfaces, with PermissionManager merely delegating.
  • User::getGlobalBlock()
  • User::isDnsBlacklist and isDnsBlacklisted (does this even need to be public?)
  • User::isLocallyBlockedProxy (does this even need to be public?)
  • User::getBlockFromCookieValue is a bot unclear. SHould not be in User, but should it be in BlockStore, or somewhere else? Should the BlockStore know about cookies?
  • Block::newFromTarget, though perhaps we want newFromUser( UserIdentity ) and newFromIP( $ip ) instead, and introduce separate methods for constructing Block objects from other kinds of block targets. If we want to leave this for later, BlockStore::newFromUser and newFromIP( $ip ) can for now call Block::newFromTarget.
  • User::getBlockStatus, and ideally any static method in Block called from there, like Block::getBlocksForIPList and Block::chooseBlock.

Considerations:

  • BlockStore should bind to UserIdentity instead of User. Any remaining references to the User class should be marked as debt to be removed. References to User should be removed from the BLock class as well, where they can be replaced with UserIdentity.
  • BlockStore should bind to PageIdentity instead of Tilte, once PageIdentity identity becomes available (see T208776). Hints against Title in parameter lists are accatable for now, returning a Title would be unacceptable.
  • Interaction with the HTTP response, in particular to get, set or clear cookies, don't belong in the storage layer. If we can't find a way to keep them out of the new service, perhaps the service is a BlockManager, not a BlockStore. Or we need both in the end.

Event Timeline

daniel created this task.Mar 15 2019, 12:10 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 15 2019, 12:10 PM
CCicalese_WMF triaged this task as Normal priority.Mar 15 2019, 5:41 PM