Ref T8989, Phurl "Visit URL" should now route to an access controller that decides if the URL is valid whether to open it, or redirect back to Phurl object. New route is local.install.com/u/1 to open link.
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T8989: Phurl objects should have a link like "Visit URL".
- Commits
- Restricted Diffusion Commit
rP809453a3e163: Ref T8989, Phurl "Visit URL" link should route through a separate controller.
- open Phurl object with invalid URL, "Visit URL" link should redirect back to object
- open Phurl object with valid URL, "Visit URL" link should open the link
- open local.install.com/u/1 for U1 with valid URL should open the link
- open local.install.com/u/1 for U1 with invalid URL should redirect to local.install.com/U1
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- phurlaccesscontroller
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 8545 Build 9861: arc lint + arc unit
Event Timeline
| src/applications/phurl/controller/PhabricatorPhurlURLAccessController.php | ||
|---|---|---|
| 17 | Hmm, why is CAN_EDIT necessary? | |
Comment Actions
Phurl shortened URL should be available to anyone who can view, even if they can't edit.
| src/applications/phurl/controller/PhabricatorPhurlURLAccessController.php | ||
|---|---|---|
| 8 | $this->getViewer() is slightly preferable in modern code. They have exactly the same effect so standardizing on one is mostly arbitrary, but, e.g., $this->getViewer() can be called from more places in a Controller so it makes it easier to break large methods apart without introducing errors. | |
| 14–18 | It should be OK to access a URL without being able to edit it. so you can remove this requireCapabilities() call. | |
| src/applications/phurl/storage/PhabricatorPhurlURL.php | ||
| 79 | This is really minor, but prefer this test instead: return isset($allowed_protocols[$uri->getProtocol()]); If $allowed_protocols has a huge number of elements for some reason, the isset() test performs better (it does a hash lookup) than the in_array() test (it has to examine each element). In this case, $allowed_protocols will probably never have a huge number of elements, but getting in the habit of using the fast test means you'll never be surprised by something being bigger than you planned for. | |
Comment Actions
Updating the view/edit capabilities required for viewing shortened url, and updating how we're locating a protocol in the allowed protocol array.
| src/applications/phurl/application/PhabricatorPhurlApplication.php | ||
|---|---|---|
| 32 | FWIW, this URL seems odd to me... is this consistent with applications? I would have expected something like /phurl/view/{$id}, but I guess we want a short URL? | |
| src/applications/phurl/application/PhabricatorPhurlApplication.php | ||
|---|---|---|
| 32 | Yeah, deferring to @epriestley, since it was his idea. | |
Comment Actions
The /U123 URI is the /view/123/ sort of URI.
The /u/1 is the redirect URI.
So you'd normally send someone to phabricator.yourcompany.com/u/1 (which will redirect).
In the future, this might be phabricator.yourcompany.com/u/picard (after aliases).
Then this might be phb.horse/u/picard (after an alternate URI site).