Implement support for creating and updating fragments from ZIPs
ClosedPublic
Actions

Authored by hach-que on Dec 6 2013, 11:34 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T4205: Implement Phragment

Commits

Restricted Diffusion Commit
rP25e7b7d53cd5: Implement support for creating and updating fragments from ZIPs

Summary

This implements support for creating and updating fragments from ZIP files. It allows you to upload a ZIP via the Files application, create a fragment from it, and have it recursively imported into Phragment. Updating that folder with another ZIP will recursively create, update and delete files as appropriate.

The logic for creating and updating fragments from files has also been centralized into the PhragmentFragment class. Directories are also now supported; a directory fragment is simply a fragment that has no patches; thus a directory fragment can be converted to a file fragment by uploading a first patch for it.

Test Plan

Uploaded ZIP files through the interface and saw all of the fragments get created and updated as expected.

Diff Detail

Branch

phragment-zip-update

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/phragment/controller/PhragmentController.php:107	XHP16	TODO Comment
Advice	src/applications/phragment/controller/PhragmentController.php:114	XHP16	TODO Comment

Unit

Tests Passed

Event Timeline

Updated for latest version of D7726

epriestley accepted this revision.Dec 7 2013, 1:35 AM

epriestley added inline comments.

src/applications/phragment/storage/PhragmentFragment.php
140	Prefer `throw`? It looks like this gets swallowed -- will the user see an upload happen which doesn't actually change anything?
156	Throw on failure? Or is this not a failure? Maybe comment what this means if it isn't?
171–173	Although this will require some gymnastics to implement, I think the top-level policy should for EDIT on a node to imply EDIT on all of its children, even if you can't VIEW them. For the purposes of this operation, we'll have to fudge the VIEW part. But we can deal with that later on.
185–191	This seems like a weird rule; I'd expect all of the children to be replaced exactly. Doesn't this mean that I can upload a zip, download it again, and end up with a different result? That seems more unintuitive and dangerous than deleting things when the users tells us to, particularly given that this operation should be recoverable/revertable eventually.
241–242	Will these still show in the browse view? Did I just miss that part? Maybe we should have a flag on the Fragment like `isDeleted` so they can be omitted from queries and browse by default?

hach-que added inline comments.Dec 7 2013, 1:49 AM

src/applications/phragment/storage/PhragmentFragment.php
140	If `php5-zip` is not installed, then the upload will behave like a normal file. The fragment will still get created for the ZIP itself (via the `updateFromFile` call above), but the children won't be imported because we have no way of reading the contents of the ZIP file.
156	`$stream` being null means that it is a directory entry in the ZIP file. Thus `$data` being null in the mappings means it's a directory entry. I'll add a comment.
185–191	At the moment the only way to convert a file fragment into a directory fragment would be to delete all of it's previous versions. If we're okay with doing that down the track, we can change this to fire only if `$mappings[$path]` is null (in which case it means the new data is also a directory and we don't want to create a new version).
241–242	These will still display in the browsing view. I thought about this for a moment, but for fragments deleted files still technically exist; but they have no file associated with them. At the moment the only way to "delete" a fragment is via uploading a ZIP, so this is probably something we'll have to deal with when it's a general operation in the UI.