A block target typically starts out as a string which is passed to BlockUtils::parseBlockTarget() which returns a tuple, the first element being a UserIdentity|string|null union, and the second element being an integer type. In various places in the code, the UserIdentity|string|null union that is returned is passed by itself into a method that calls parseBlockTarget() again, despite it not being idempotent. Sometimes, the "#" prefix for autoblock IDs is re-added before passing the target to parseBlockTarget() -- this gets it closer to a correct round-trip.

I logged 10 calls to parseBlockTarget() during an API block action.

BlockUtils::parseBlockTarget() has some UI level features which don't really belong in backend code, like trimming spaces. The "#" prefix is properly a UI feature.

The union itself is awkward and the relevant code spends a fair few lines figuring out what is going on with it.

So I propose a BlockTarget class hierarchy.

Proposal:

• Add BlockTargetFactory
• BlockTargetFactory::newFromString() replaces BlockUtils::parseBlockTarget()
• BlockTarget::isValid() instead of BlockUtils::validateTarget(). This is pretty lightweight and doesn't currently depend on services. If it did depend on services, the factory could just inject a validity flag, there isn't really a need to defer validity checking.
• There will be nothing left in BlockUtils and it can be deprecated.
• DatabaseBlockStore methods that take a union will be migrated to take a BlockTarget instead. BlockTarget will temporarily be added to the union, with string|UserIdentity being hard-deprecated.
• BlockUser and UnblockUser constructor parameters will be migrated.
• Add Block::getTarget().
• Callers would benefit from the following convenience methods: toString(), toHex(), toHexRange(), getUserPage(), getUserIdentity().

I am leaning towards instanceof with many small subclasses, as an alternative to nullable accessors. Consider:

$page = $target->getUserPage();
if ( $page ) {
   ...
}

versus

if ( $target instanceof BlockTargetWithUserPage ) {
    $page = $target->getUserPage();
}

The latter has short and simple getter implementations. Static analysis on the calling code ensures the getter is safe to call. It's a conditional downcast instead of an error check. IP address accessors like toHex() would not need to be nullable in such a scheme, since the factory validates the address before creating the object.