Whatever we use as name/alias/code should have a UNIQUE KEY on it.

description and longURL be LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, not VARCHAR(2047).

This is correct/desirable.

This one probably won't exist (just /U123 instead).

We probably don't need sequence in this app

Prefer to simplify this as:

$id = $request->getURIData('id');
$is_create = !$id;

Probably unused.

Probably just /U123.

Prefer $view_policy / $edit_policy for consistency.

This needs ->setURL('/U1234'). We could also fix RedirectResponse to throw a better error if you forget to do this.

Prefer handleRequest(AphrontRequest $request) + $id = $request->getURIData('id'); to simplify this stuff slightly.

Prefer $this->getViewer().

(You could implement $url->getMonogram() to return U123 if you want to simplify this a little.)

Oh, you already did. Consider using getMonogram() and getURI() to simplify slightly.

Prefer $this->getViewer().

getApplicationURI() prepends the phurl/ part, this should just be /U123 (not `getApplicationURI()).

I'd expect this to end up pointing at /phurl/phurl/, the leading phurl should be redundant.

Debugging leftovers.

I think you can just omit these methods, you aren't required to implement them if you don't have any behaviors you need to activate.

You shouldn't need this, the id column is automatically orderable.

You should be able to implement this as return $this->loadStandardPage($this->newResultObject()) now.

In modern code, implement buildWhereClauseParts(AphrontDatabaseConnection $conn) instead. This is a similar method but gets you more stuff "for free".

Prefer to test the explicitly against !== null. For example:

if ($this->ids !== null)

This makes $query->withIDs(array()) fail in a relatively obvious way, instead of loading all results (which can cause non-obvious infinite loops / hangs once an install has millions of objects).

Particualrly, you get this (and some other similar auto-includes) "free" when you implement buildWhereClauseParts() instead of buildWhereClause().

You should be able to just omit this, since this Query class doesn't do anything unusual. The default behavior will work properly.

You should be able to omit this as unused.

Seems fine/desirable to show the order field in this app?

Unused.

This isn't correct -- it finds URLs with the given PHIDs (always empty), not URLs authored by specific users.

In modern code, just omit this.

Instead, load handles here:

$handles = $viewer->loadHandles(mpull($url, 'getUserPHID'));

Unused.

Omit render() if possible, not sure if you can.

You can omit this, this is the default behavior.

MarkupInterface probably isn't worth implementing. This change doesn't do anything with it, in particular.

These properties don't actually exist.

Yeah, I'd just junk this.

Since we have full edit policy support, maybe just no automatic capabilities for these? It doesn't seem to me like the author of a URL has a strong, enduring relationship which should give them permanent control over it.

This PHID is the PHID of the URL, not of the author, so the policy is meaningless.

Describing the wrong application.

This will auto-subscribe the URL to itself, which doesn't make much sense.

If we want to give authors long-term, enduring relationships to URLs they create, this should do return ($phid == $this->getAuthorPHID()). If not, just return false;.

Property doesn't exist.

Add period for consistency.

Since this transaction is always hidden, you could just omit this branch.

(Remove sooner or later.)

Should have a trailing slash.

(Cruft.)

Remove.

Just use $id.

Or $url->getURI() now.

I think these are copy/pasted out of something (Calendar?) and not used in this workflow.

Or getMonogram() / getURI().

In the else case here, I think most applications just add an addTextCrumb(pht('Create URL')) or something like that.

(You could implement $url->getMonogram() to return U123 if you want to simplify this a little.)

Or $this->getViewer().

Unused.

Unused.

Or $this->getViewer().

Maybe put this after "original URL"? It will cause stuff like "Projects" and "Subscribers" to populate when you invoke it, keeping them above "Description", which is likely desirable.

I favor making this optional at some point, but not important for now.

Cruft / TODO?

Could simplify slightly with getMonogram()

"$autho" should be "$author"

Cruft?

Prefer explicit !== null.

Consider "Author" language for consistency.

Since we let you query by authorPHID, you could also add a key here to improve the performance of that query:

'key_author' => array(
  'columns' => array('authorPHID'),
),

That will make queries with a WHERE authorPHID IN (...) part (and no better key) faster.

Cruft / TODO?