Page MenuHomePhabricator

Factor rev_deleted logic out of the API modules
Open, MediumPublic

Description

The code bit used to determine the bitmask for rev_delete when querying contributions is duplicated in three different API modules: ApiQueryAllRevisions, ApiQueryRevisions, and ApiQueryUserContribs.

Hence, it should be factored out of the API and moved somewhere else, probably somewhere in the Revision namespace as a static method.

Event Timeline

daniel moved this task from Triage Meeting Inbox to Icebox on the Platform Engineering board.
Pchelolo raised the priority of this task from Low to Medium.Nov 5 2019, 3:57 PM
Pchelolo added a subscriber: Pchelolo.

This also affects the new core REST API project. Since this logic doesn't currently have a common place, it keeps being duplicated in new REST API handlers as well.

Since this logic is pretty much an inverse of RevisionRecord::userCanBitfield, it might belong in RevisionRecord as well, like 'getUserCanBitfield'. However, that's not a very DI-friendly way, and userCanBitfield method already has existing TODO comments about avoiding the global state.

Other natural place to put it is RevisionStore. However the method requires a PermissionManager, and injecting a PM into RevisionStore will create a cyclic dependency.

Putting both methods into PermissionManager doesn't quite solve the problem either. getUserCanBitfield is intended to be used for querying revisions, so it will need to be used inside the RevisionStore, which would create a cyclic dependency yet again.

Given that It falls under modularization, maybe @daniel already has some ideas?

this logic should be folded into RevisionStore::getQueryInfo() or an associated method. RevisionStore provides encapsulation for the schema of the revision table, so that's where this logic should live.

Other natural place to put it is RevisionStore. However the method requires a PermissionManager, and injecting a PM into RevisionStore will create a cyclic dependency.

This is correct, and it's already the case, via RevisionRecord::userCanBitfield(). There are two things that can and shoule be done to address this:

  • PermissionManager shouldn't depends on RevisionStore.
  • The static RevisionRecord::userCanBitfield() method should be replaced by a callback that is injected into RevisionStore.

Change 549123 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Make PermissionManager not depend on RevisionLookup.

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

Now that we have Authority, we can implement a static method on RevisionRecord and use it.

Something like

       /**
	 * Get a bitmask to apply to `rev_deleted` column appropriate for permissions
	 * given $performer has.
	 *
	 * @since 1.36
	 * @param Authority $performer user on who's behalf the revision is viewed
	 * @return int
	 */
	public static function getRevDeletedBitmask( Authority $performer ): int {
		if ( !$performer->isAllowed( 'deletedhistory' ) ) {
			$bitmask = RevisionRecord::DELETED_USER;
		} elseif ( !$performer->isAllowedAny( 'suppressrevision', 'viewsuppressed' ) ) {
			$bitmask = RevisionRecord::DELETED_USER | RevisionRecord::DELETED_RESTRICTED;
		} else {
			$bitmask = 0;
		}
		return $bitmask;
	}

Change 549123 abandoned by Ppchelko:
[mediawiki/core@master] Make PermissionManager not depend on RevisionLookup.

Reason:

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

I just realized that everything from this task also applies to the logic behind recentchanges.rc_deleted (relevant code, duplicated e.g. here). In fact, I think the logic would be pretty much the same.