Page MenuHomePhabricator

Block::getTarget() can return a User object or a string that is an invalid user name
Closed, ResolvedPublic

Description

The fact that $this->getTarget() sometimes returns a User object and sometimes doesn't is confusing and caused T208398. It would be nice if there was a getTargetUser() method, or if the hacks that rMWeab725e3f16d: Follow-up d67121f6d: Blocks can apply to non-User objects too copied around were in some way centralized or made less necessary.

At a higher level, the concept of a block target and the concept of an "actor" aren't quite the same, but seem related. An actor can be a logged-in user or an IP user. A block target can be a logged-in user, or a single IP, or an IP range, or a block ID (for autoblocks).

Event Timeline

Catrope created this task.Oct 31 2018, 10:41 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 31 2018, 10:41 PM
Anomie added a subscriber: Anomie.Nov 1 2018, 2:39 PM

A getTargetUser() method would have to return null for range blocks and auto-blocks, so that seems unlikely to be much of an improvement over the status quo. It might be easier to realize the right thing to do, I suppose.

After reviewing the changes in rMWeab725e3f16d: Follow-up d67121f6d: Blocks can apply to non-User objects too, nothing in there really makes sense.

  • The existing instance near line 770 was added to maintain behavior from old code when updating for actors. After closer analysis, it may as well just return if the type isn't TYPE_USER, since autoblocks only apply to that type. The one caller already only calls it for TYPE_USER blocks.
  • The existing instance near line 1598 isn't dealing with a target at all.
  • The instance added at line 1813 seems to be just wrong, you're not going to get a sensible result trying to call $user->getTalkPage() for a range or auto-block. What you really need there for it to work right in the face of range blocks and autoblocks is to pass in the User that's actually trying to make the edit.

Change 471018 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Block: Clean up handling of non-User targets

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

daniel moved this task from Inbox to Watching on the TechCom board.
brion added a subscriber: brion.Nov 8 2018, 6:14 AM

Change 471018 merged by jenkins-bot:
[mediawiki/core@master] Block: Clean up handling of non-User targets

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

dbarratt closed this task as Resolved.Nov 19 2018, 11:57 PM
dbarratt claimed this task.
dbarratt moved this task from Review to Done on the Anti-Harassment (AHT Sprint 33) board.