Page MenuHomePhabricator

Where should page move permissions go?
Open, Needs TriagePublic

Description

So I've been trying to write some tests for MovePage. In doing so, I found out that the error codes returned by the current code don't make a lot of sense, and to fix that, I started refactoring. I had to go through a number of iterations until finally I got something I thought I was happy with. Then I discovered I had created a circular dependency between MovePage and PermissionManager.

So the question is: where should the logic of Title::isMovable() live? I wanted to put it in MovePage, but it turns out PermissionManager uses it, and MovePage naturally has to use PermissionManager as well, so we have a circular dependency. Is PermissionManager supposed to also deal with non-permissions-related reasons for why an action might fail? I suppose it should, because it's used to decide whether to display UI to do the action, and we don't want to do that for an impossible action. But then where do we draw the line? Which checks should live in PermissionManager and which elsewhere? If we want them all to live in PermissionManager, what do we do about things like self-move that aren't detectable by looking at only one target at a time?

Well, hopefully I'll at least be able to submit my tests tomorrow, if not my code. The expected results will be somewhat silly, though.