Implement snapshots in Phragment
ClosedPublic
Actions

Authored by hach-que on Dec 8 2013, 7:10 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T4205: Implement Phragment
T4212: Create snapshots in Phragment

Commits

Restricted Diffusion Commit
rP8bb6e807f097: Implement snapshots in Phragment

Summary

Ref T4212. This implements snapshots in Phragment, which allows you to take a snapshot of a fragment at a given point in time, and download a ZIP of the snapshot as it was in this state.

There's also functionality for deleting and promoting snapshots. You can promote a snapshot to either the latest version or any other snapshot of the fragment.

Test Plan

Clicked around, took some snapshots, promoted them to different points and deleted snapshots. Also downloaded ZIPs of the snapshots and saw the right versions coming through for all the files downloaded.

Diff Detail

Branch

snapshot

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/phragment/controller/PhragmentController.php:145	XHP16	TODO Comment
Advice	src/applications/phragment/controller/PhragmentController.php:153	XHP16	TODO Comment
Advice	src/applications/phragment/controller/PhragmentSnapshotViewController.php:111	XHP16	TODO Comment
Advice	src/applications/phragment/controller/PhragmentSnapshotViewController.php:119	XHP16	TODO Comment
Advice	src/applications/phragment/controller/PhragmentSnapshotViewController.php:127	XHP16	TODO Comment

Unit

Tests Passed

Event Timeline

A couple of notes inline (and one thought on T4212) but nothing substantive.

resources/sql/patches/20131208.phragmentsnapshot.sql
6–7	Thinking out loud, but it might be nice to have an optional description field here, too.
9	This should have: UNIQUE KEY `key_name` (primaryFragmentPHID, name) The `name` may need to be reduced to 192-ish characters to accommodate this index. If you want to allow arbitrarily long names instead, we can: Add `nameIndex CHAR(12) NOT NULL COLLATE latin1_bin` Change `name` to `LONGTEXT`. Make the key UNIQUE KEY `key_name` (primaryFramgentPHID, nameIndex). Override `save()` and do `$this->nameIndex = PhabricatorHash::digestForIndex($this->name);` When querying by `name`, digest the string first. This is a bit more complicated but allows names to have unlimited length.
src/applications/phragment/controller/PhragmentSnapshotCreateController.php
40–42	Eventually, we should use `phutil_escape_uri_path_component()` / `phutil_unescape_uri_path_component()` to remove this restriction.
51–55	With the unique key, it's cleaner to let this go through and then detect it by catching `AphrontQueryDuplicatKeyException`, since that prevents race conditions where two people try to create a snapshot at the same time, both get past this check, and then one loses the race later on. Eventually, it would be nice to just prompt the user here: X already exists, promote it? [cancel] [ok]
src/applications/phragment/controller/PhragmentSnapshotDeleteController.php
27–39	It's slightly preferable to override Snapshot->delete() and do this logic there. This helps make sure that some future `bin/destroy <phid>` command will work correctly, for example. Check out the delete() method on PhabricatorUser or PhabricatorRepository for examples. (Using the very-low-level `load()` methods in this case is OK.)
58–59	It would eventually be nice to mark this as `deleted` instead and still provide access to, e.g., see who deleted it, but we can easily graft that on later.
src/applications/phragment/controller/PhragmentSnapshotPromoteController.php
4	At some point, it might be cleaner to merge this with the "create" interface, and have one combined "create or move" sort of thing, but we don't have any similar UIs and I'm not immediately sure how to lay it out so it isn't confusing.
src/applications/phragment/query/PhragmentSnapshotChildQuery.php
4	It miight be cleaner to get rid of this and put everything on `SnapshotQuery`.
11–12	Conventionally, prefer `need` to `needs`.
src/applications/phragment/query/PhragmentSnapshotQuery.php
11	...e.g., a `needChildren()` here.

(I also like this snapshotting feature quite a lot, it seems like it does a good job of being really easy to use and understand while still being very powerful.)

hach-que added inline comments.Dec 8 2013, 9:16 PM

resources/sql/patches/20131208.phragmentsnapshot.sql
6–7	I'll add a `description LONGTEXT NULL COLLATE utf8_bin` field but not expose it in the UI for now. I can send through another diff later to make use of it.
9	I think it's fine to have names limited to 192 characters.
src/applications/phragment/controller/PhragmentSnapshotPromoteController.php
4	Yeah there's a lot of similar-but-not-quite-the-same logic going on here. It's not identical enough that it can be combined because the promote logic has to do drastically different things based on whether it's being promoted up to the latest version or another snapshot.
src/applications/phragment/query/PhragmentSnapshotChildQuery.php
11–12	Won't this conflict with the function `needFragments()` then?

PHP's methods and properties are in different namespaces

derpdog

$ cat derp.php 
<?php

final class derp {

  const derp = 'a';
  public $derp = 'b';
  public function derp() {
    return 'c';
  }

}

$derp = new derp();
echo derp::derp."\n";
echo $derp->derp."\n";
echo $derp->derp()."\n";

$ php -f derp.php 
a
b
c