Phriction - validateTransactions that need parent ancestry to complete successfully
ClosedPublic
Actions

Authored by btrahan on Dec 11 2014, 9:45 PM.

Details

Reviewers

Maniphest Tasks

T6682: Exception when moving wiki document
T6651: Wiki allows creation of document under empty root, but then won't save it

Commits

Restricted Diffusion Commit
rP905fc217b80c: Phriction - validateTransactions that need parent ancestry to complete…

Summary

Fixes T6651, T6682. Since policy is defined by ancestry, you can't make things outside the core tree.

An alternative fix would be to automagically stub everything in these cases. This has potential negative policy implications - consider making a public document with several levels of depth that automagically stubs out its ancestry as public - and additionally the PhabricatorApplicationTransactionEditor framework would make this very tricky code (i.e. you are expected to validateTransactions in said hook *and* return an error if things aren't valid and not do some automagic stubbing, etc.)

Test Plan

tried to move a doc from location/that/exists to locationz/thatz/dontz/existz/ and got an error message with links to each missing doc. tried to create a doc at locatonz/thatz/dontz/existsz/ and got an error message with links to each missing doc.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

btrahan updated this revision to Diff 26360.Dec 11 2014, 9:45 PM

btrahan added tasks: T6651: Wiki allows creation of document under empty root, but then won't save it, T6682: Exception when moving wiki document.

btrahan retitled this revision from to Phriction - validateTransactions that need parent ancestry to complete successfully.

btrahan updated this object.

btrahan edited the test plan for this revision. (Show Details)

btrahan added a reviewer: epriestley.

Herald added subscribers: epriestley, Korvin. · View Herald TranscriptDec 11 2014, 9:45 PM

btrahan added inline comments.Dec 11 2014, 9:46 PM

src/applications/phriction/editor/PhrictionTransactionEditor.php
527–558	I can consolidate this block to a function... its only change is create => move.

make a validateAncestry helper function to reduce the copy and paste action to nil

epriestley accepted this revision.Dec 12 2014, 12:31 AM

epriestley edited edge metadata.

epriestley added inline comments.

src/applications/phriction/editor/PhrictionTransactionEditor.php
652–661	Piecewise strings like this can't be translated in the general case -- you should ideally pass a constant instead, and then do something like: switch ($operation) { case MOVE: return pht('entire string with move verb'); case CREATE: return pht('entire string with create verb'); }

This revision is now accepted and ready to land.Dec 12 2014, 12:31 AM

use a constant and construct strings properly

Closed by commit rP905fc217b80c: Phriction - validateTransactions that need parent ancestry to complete… (authored by btrahan, committed by btrahan). · Explain WhyDec 12 2014, 7:35 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

src/

applications/

phriction/

editor/

PhrictionTransactionEditor.php

79 lines

Diff 26373

View Options

src/applications/phriction/editor/PhrictionTransactionEditor.php

Phriction - validateTransactions that need parent ancestry to complete successfullyClosedPublicActions