Page MenuHomePhabricator

Turn EditEntity interactor into a service
Closed, DeclinedPublic

Description

The EditEntity interactor currently has a very unclear usage pattern. It should be refactored step by step, and turned into (or replaced by) a service that provides the following method:

updateEntity( EntityId $id, ChangeOp $op, int $baseRevId, User $user, $token )

That method would load the base revision (the current revision, if $baseRevId is 0), check for edit conflicts, validate the ChangeOp, apply the CHangeOp, check permissions, token, and rate limits, and save the change, using the summary derived from the ChangeOp. User watchlist updates could be intergrated, but should probably be done by a separate service, exposing a method like updateWatchlist( EntityId $id, User $user, $watch ).

To get to that point, the following steps are relevant:

  • Make EditEntity::attemptSave take the new EntityDocument as a parameter
  • Make calling code use EditEntity::getBaseRevision to load the base revision for modification
  • Make calling code use EditEntity::getCurrentRevision to load the current revision for ChangeOp validation.
  • Factor out precondition checks to a separate service, for composition (check at least permissions, toke, and rate limit)
  • Factor out watchlist update (see above)
  • Create a EntityUpdater service that defines updateEntity
  • Implement load/modify/store cycle in updateEntity. This includes edit conflict detection/resolution and ChangeOp validation.
  • Change all code that uses EditEntity to use EntityUpdater. Thsi implies that all modifications are done via ChangeOps.

Once all entity updates are done via ChangeOps, we can replace the diff-and-patch approach to conflict detection/resolution by something more elegant, based on ChangeOps::getConflicts( $baseRev, $currentRev ); see T126231: [RFC] Detect edit conflicts in ChangeOp instead of using diff-and-patch in EditEntity.

Perhaps it even becomes possible to base the update of secondary data (links tables) on ChangeOps directly, see T59750: [Task] Use diff information to optimize database updates when saving an entity.

Event Timeline

Addshore subscribed.

Going to close this as if we look at this area again we will write a fresh ticket, but I think reading this now will just be confusing as much of this code has changed and move around since 2017