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.
Description
Description
Details
Details
Customize query in gerrit
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Urbanecm | T250737 Special:Block should not play the role of a central blocking service/utility | |||
| Resolved | matmarex | T177329 The code in HTMLUserTextField, as used on Special:Block, would incorrectly check IP ranges (if it worked) | |||
| Resolved | Urbanecm | T263189 Move SpecialBlock::validateTarget to a BlockUtils service | |||
| Resolved | DannyS712 | T263249 Move AbstractBlock::parseTarget to the BlockUtils service |
Event Timeline
Comment Actions
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
Comment Actions
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.
Comment Actions
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
Comment Actions
Change 628427 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Introduce BlockUtils service
Comment Actions
Change 628427 merged by jenkins-bot:
[mediawiki/core@master] Introduce BlockUtils service
Comment Actions
Change 631990 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] BlockUtils: remove unused params
Comment Actions
Change 631991 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Do not call SpecialBlock from BlockUser
Comment Actions
Change 631990 merged by jenkins-bot:
[mediawiki/core@master] BlockUtils: remove unused params, rename methods
Comment Actions
Change 631991 merged by jenkins-bot:
[mediawiki/core@master] Do not call SpecialBlock from BlockUser
Comment Actions
Change 631992 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@master] Hard deprecate SpecialBlock::validateTarget
Comment Actions
Change 631992 merged by jenkins-bot:
[mediawiki/core@master] Hard deprecate SpecialBlock::validateTarget