Page MenuHomePhabricator

Implement "replace" transactions in Pholio without "applyInitialEffects"
ClosedPublic

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

Details

Summary

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

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Dec 20 2018, 9:33 PM
epriestley requested review of this revision.Dec 20 2018, 9:35 PM
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 accepted this revision.Dec 24 2018, 9:14 PM
amckinley added inline comments.
src/applications/pholio/controller/PholioMockEditController.php
205

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
epriestley added inline comments.Dec 28 2018, 7:57 AM
src/applications/pholio/controller/PholioMockEditController.php
205

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.