Page MenuHomePhabricator

Move SpecialBlock::validateTarget to a BlockUtils service
Closed, ResolvedPublic


per discussion with @Tchanders over IRC and T250737, we should move SpecialBlock::validateTarget to a service, and call it from special pages, BlockUser (T189073) and elsewhere where it is needed.

Event Timeline

Hmm, so I took a look and doing this with dependency injection

SpecialBlock::validateTarget includes static calls to:

  • SpecialBlock::getTargetAndType
  • SpecialBlock::checkUnblockSelf
  • IPUtils::isIPv4
  • IPUtils::isIPv6
  • Status::newGood

The IPUtils and Status calls can remain the same, but the new service shouldn't depend on SpecialBlock

SpecialBlock::checkUnblockSelf just creates a new BlockPermissionChecker from the factory and calls checkBlockPermissions

SpecialBlock::getTargetAndType relies on DatabaseBlock::parseTarget, which does not exist as an override and so really uses AbstractBlock::parseTarget. That method uses User::newFromIdentity and User::newFromName. I was going to suggest that parseTarget also become part of the new utils service with a UserFactory injected, but BlockPermissionChecker::__construct includes a call to AbstractBlock::parseTarget, so that would create a circular dependency.

So, I suggest considering creating two services, one to replace AbstractBlock::parseTarget and be injected into the BlockPermissionChecker and into the second, and a second to replace SpecialBlock::getTargetAndType and SpecialBlock::validateTarget

getTargetAndType doesn't really do anything extra without a context, and it shouldn't be used outside of the special page. I think we should work at one step at a time, with this very task being about validating a (already parsed) target. I think we should replace only validateTarget, which cannot be replaced without code duplication, and leave discussion about AbstractBlock refactoring for later - it doesn't really cause any issue AFAICS.

Thanks for looking into this @DannyS712.

It's a bit odd that validateTarget calls checkUnblockSelf, so I'm inclined to think we should remove that call, and move parseTarget to the BlockUtils service as you suggest. Callers of validateTarget should call the BlockPermissionChecker separately.

That way, validateTarget only does actual target validation, i.e. checking that the user exists, checking the IP/range is valid, etc.

Here's a task for moving parseTarget after this task - T263249

Change 628427 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Introduce BlockUtils service

Change 628427 merged by jenkins-bot:
[mediawiki/core@master] Introduce BlockUtils service

Change 631990 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] BlockUtils: remove unused params

Change 631991 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Do not call SpecialBlock from BlockUser

Change 631990 merged by jenkins-bot:
[mediawiki/core@master] BlockUtils: remove unused params, rename methods

Change 631991 merged by jenkins-bot:
[mediawiki/core@master] Do not call SpecialBlock from BlockUser

Change 631992 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Hard deprecate SpecialBlock::validateTarget

Change 631992 merged by jenkins-bot:
[mediawiki/core@master] Hard deprecate SpecialBlock::validateTarget