Ref T4205. This is an initial implementation of Phragment. You can create and browse fragments in the system (but you can't yet view a fragment's patches / history).
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4205: Implement Phragment
- Commits
- Restricted Diffusion Commit
rP4c143ad3b2c4: Phragment v0
Clicked around and created fragments.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Some minor inlines.
How are you planning to version Fragments?
I think requiring N queries to load N parents is a pretty big weakeness. If we store a path instead (like Phriction does) we can load parents in one query. In particular, the visibility of x/y/z/ should depend on the visibility of x/y/, so we must load all parents to load any Fragment. By storing both path and depth, we can query children/grandchildren like Phriction does, too. Does that seem reasonable? Are there reasons parentPHID is better? I'm worried we're going to end up doing thousands of queries to list a page of, e.g., recently updated fragments with this approach.
| src/applications/phragment/controller/PhragmentBrowseController.php | ||
|---|---|---|
| 78 | You should be abel to do this with $header->setPolicyObject($fragment) | |
| src/applications/phragment/storage/PhragmentPatch.php | ||
| 53–64 ↗ | (On Diff #17444) | As below, passing null will fatal. |
| 79 ↗ | (On Diff #17444) | It's not possible for $old or $new to be null; PHP will raise a typehint warning which we'll convert into an exception when we enter the method. You can add = null in the method signature to allow it, at the cost of making the signature a bit awkward. |
| 88–113 ↗ | (On Diff #17444) | Is this cleaner as: if ($old_hash === $new_hash) {
return null;
}
if ($old_hash !== self::EMTPY_HASH) {
$old_content = $old->loadFileData();
} else {
$old_content = "";
}
if ($new_hash !== self::EMTPY_HASH) {
$new_content = $new->loadFileData();
} else {
$new_content = "";
}...maybe? |
Comment Actions
Fragments themselves can't be moved or renamed, so it seems perfectly fine to permanently store the path and depth in the fragment itself and the load based on that. There's no reasons to use parentPHID as far as I can see.
Patches are the versioning for a fragment; a fragment will always have at least one patch (going from hash 00000... to the hash of the current file). When a fragment is updated, we create a new patch to go from the old file to the new file, and update the fragment's hash and file PHID to point to the new file.
Comment Actions
I've retained the word "patch" for now since I don't want to use the word "version" (since that implies some human associated versioning). I'm open to better suggestions on what to call the fragment at each point in time.
Comment Actions
Some legitimate catches on database keys, and some minor quibbling.
| resources/sql/patches/20131206.phragment.sql | ||
|---|---|---|
| 7 | Does giving these an owner really make sense? They seem more like a collective resource (like a repository) than a personal one (like a Paste or Revision). | |
| 13 | Add: UNIQUE KEY key_path (path) This lets us find fragments by path, and ensures we don't have two with the same path. It is possible (path, depth) or an additional (depth, path) key would also benefit some queries, but that one's murkier to me; we can wait until we have data. | |
| 20 | Maybe make this nullable for the initial write. There's no practical consequence, but null is a little cleaner in concept. | |
| 24 | Add: UNIQUE KEY `key_phid` (phid) The key_version should be: (fragmentPHID, sequence) ...I think? Two reasons on that:
| |
| src/applications/phragment/controller/PhragmentBrowseController.php | ||
| 20 | Equivalent to last($parents)? (Except I don't remember if last() returns false or null; it's probably false for consistency with end()). | |
| 61–65 | pht | |
| src/applications/phragment/storage/PhragmentFragment.php | ||
| 33 | Can be simplified slightly with last(), probably slightly more with basename(). | |
| resources/sql/patches/20131206.phragment.sql | ||
|---|---|---|
| 7 | It's mainly for policies so that there's always an owner who can recover it. Maybe it's more suitable to just give administrators implicit access to view and edit fragments, what do you think? | |
| 13 | Will fix. | |
| 20 | In a later diff I reverse the logic here so that latestVersionPHID can be null on the fragment and the fragment gets saved before the first patch is assigned. Making latestVersionPHID nullable has to be done to support directory fragments, so I can bring that change forward into this diff if required. | |
| 24 | Will fix. | |
| src/applications/phragment/controller/PhragmentBrowseController.php | ||
| 20 | Will fix. | |
| src/applications/phragment/controller/PhragmentBrowseController.php | ||
|---|---|---|
| 20 | It's literally a wrapper around end(), so it will return false on an empty array. | |
Comment Actions
Administrators can recover any object with bin/policy unlock, and ApplicationTransactions generally prevent you (partly in practice, partly in theory) from making edits which lock you out of objects. This is still possible, but not in a single step: you have to, e.g., set the policy to "members of project X", and then leave that project.
So far, these methods seem to be sufficient. If we get more feedback later on that they aren't, we'll have to deal with them globally anyway (maybe "admins can see anything" flag), so I think it's fine to not plan for this case.
Comment Actions
(And don't worry about shipping changes around across diffs, I don't see much value in it and it's a pain to do.)