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

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

Details

Reviewers

amckinley

Maniphest Tasks

T11351: Move Pholio to EditEngine and modern APIs

Commits

rP741fb747db42: Implement "replace" transactions in Pholio without "applyInitialEffects"

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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Dec 20 2018, 9:33 PM

Harbormaster completed remote builds in B21402: Diff 47549.Dec 20 2018, 9:35 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)

epriestley added a child revision: D19926: Implement Pholio file add/remove transactions without "applyInitialEffects".Dec 20 2018, 9:53 PM

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.