Page MenuHomePhabricator

Ignore the no-block block use case in Title::checkUserBlock()
Closed, ResolvedPublic

Description

Problem
Currently, we preserve the use case of Title::checkUserBlock() returning a block error from a hook via UserIsBlockedFrom even if the user does not have a block (since that hook allows an extension to override whether a user is blocked from a page, regardless if the have a block or not).

However, in Title::checkPermissionHooks() there are many other ways to do this way before the system gets to this point.

Because we are preserving this behavior unnecessarily, we have unnecessary overhead on the database by going through the rest of the blocking logic.

Solution
Remove this use case (by returning early if the user doesn't have a block in Title::checkUserBlock()) , but still let the use case remain in User::isBlockedFrom(). Since there is no known usage of using this hook to enforce a block, even though none exist, this shouldn't cause any problems.

Event Timeline

dbarratt created this task.Feb 16 2019, 6:04 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 16 2019, 6:04 AM

Change 490961 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/core@master] Return early in Title::checkUserBlock() if user does not have a block.

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

dbarratt claimed this task.Feb 16 2019, 6:09 AM
dbarratt moved this task from Ready to Review on the Anti-Harassment (Dalet — ד) board.

The only supported hook appears to be in LiquidThreads and returns straight away if there is no block.

Change 490961 merged by jenkins-bot:
[mediawiki/core@master] Return early in Title::checkUserBlock() if user does not have a block.

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

dbarratt closed this task as Resolved.Feb 25 2019, 2:42 PM
dbarratt moved this task from Review to Done on the Anti-Harassment (Dalet — ד) board.

I don't see how this can be tested, other than a regression test, so I will boldly skip QA.