Allow a domain other than the install domain to serve as a short Phurl domain
ClosedPublic
Actions

Authored by lpriestley on Nov 9 2015, 6:12 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T8995: Allow a domain other than the install domain to serve as the "short" Phurl domain

Commits

Restricted Diffusion Commit
rPd8111f828f4d: Allow a domain other than the install domain to serve as a short Phurl domain

Summary

Ref T8995, config option for Phurl short domain to share shortened URL's

Test Plan

Configure Phurl short domain to something like "zz.us"
Navigate to zz.us; get 404
Navigate to zz.us/u/3 or zz.us/u/alias where U3 is an existing Phurl; redirect to correct destination

Diff Detail

Repository

rP Phabricator

Branch

phurlexternalurl

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 8729
Build 10139: Run Core Tests
Build 10138: arc lint + arc unit

Event Timeline

lpriestley updated this revision to Diff 34918.Nov 9 2015, 6:12 PM

lpriestley retitled this revision from to Allow a domain other than the install domain to serve as a short Phurl domain.

lpriestley updated this object.

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

lpriestley added a reviewer: epriestley.

lpriestley added a task: T8995: Allow a domain other than the install domain to serve as the "short" Phurl domain.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptNov 9 2015, 6:12 PM

Herald added a subscriber: Korvin. · View Herald Transcript

lpriestley added inline comments.Nov 9 2015, 6:13 PM

src/applications/config/option/PhabricatorPhurlConfigOptions.php
27	Not sure what a good default here is.
31	Also not sure what's a useful example.
src/applications/phurl/controller/PhabricatorPhurlURLAccessController.php
36–39	Is this still necessary?

This generally looks good to me.

For consistency, prefer put the access-modifying methods (shouldRequireLogin(), shouldAllowPublic()) at the top of the controllers rather than at the bottom.

src/aphront/site/PhabricatorShortSite.php
6	Plural of URL is probably just "URLs"?
10	This site should have a unique priority so its execution order is unambiguous. It currently shares the same priority with `PhabricatorResourceSite`. You can use Config → Sites to see other sites. `2500` would be a reasonable priority for this site: it should be weaker than CDN configuration, but stronger than Phame blogs.
20	We should also have a check to make sure Phurl is installed. See `PhameSite` for an example of how to check if an app is installed (that one checks for Phame).
src/applications/config/option/PhabricatorPhurlConfigOptions.php
11	Maybe just "Options for Phurl.", the next thing we put here might not directly relate to short domains.
19	I think this should be `phurl`, as it will currently conflict with the existing `core` group. The Config list should show a new config group for "Phurl".
24	Prefer `null` as the default, not `false`.
27	The default should just be `null`. You should be able to omit this call completely, I think.
31	Maybe `https://some-very-short-domain.museum/`
src/applications/phurl/application/PhabricatorPhurlApplication.php
56–57	Since you don't actually care which one matches, you could just use a single rule here and simplify the `ShortURLController`.
src/applications/phurl/controller/PhabricatorPhurlURLAccessController.php
36–39	It's not necessary for this change, but it's desirable so that the "Public" policy works. Without this, public Phurls will prompt users to log in, even though they do not need to.