Page MenuHomePhabricator

Implement "replace" transactions in Pholio without "applyInitialEffects"

Authored by epriestley on Dec 20 2018, 9:33 PM.



Depends on D19923. Ref T11351. Currently, this transaction takes an Image as the newValue and uses some magic to reduce it into PHIDs by the time we're done.

This creates some problems today where I'd like to get rid of applyInitialEffects for MFA code. In the future, it creates a problem becuase there's no way to pass an entire Image object over the API.

Instead, create the Image first, then just provide the PHID. This is generally simpler, will work well with the API in the future, and stops us from needing any applyInitialEffects stuff.

Test Plan

Replaced images in a Pholio mock.

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

epriestley retitled this revision from Implement "replace" transactions in Pholio without "willApplyTransactions" to Implement "replace" transactions in Pholio without "applyInitialEffects".Dec 20 2018, 9:50 PM
epriestley edited the summary of this revision. (Show Details)
amckinley added inline comments.

This is weird; why did we explicitly open a transaction here? Was the old, editor-less code updated without removing the transaction stuff?

This revision is now accepted and ready to land.Dec 24 2018, 9:14 PM

I think this might have been trying to make sure that if the Editor failed for whatever reason, we didn't leave Image objects in the database, since applyInitialEffects() may not always have happened inside the main transaction. The writes moved inside a transaction by T13054 (and probably some time before that) and this explicit transaction management was obsoleted somewhere along the way, although it would be hard to say exactly where.

This revision was automatically updated to reflect the committed changes.