Page MenuHomePhabricator

Ref T8989, Phurl "Visit URL" link should route through a separate controller.
ClosedPublic

Authored by lpriestley on Nov 2 2015, 7:39 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 24, 5:07 AM
Unknown Object (File)
Thu, Dec 19, 11:40 PM
Unknown Object (File)
Sat, Dec 14, 4:47 PM
Unknown Object (File)
Fri, Dec 13, 9:56 PM
Unknown Object (File)
Wed, Dec 11, 7:20 AM
Unknown Object (File)
Mon, Dec 9, 4:11 PM
Unknown Object (File)
Mon, Dec 2, 4:12 AM
Unknown Object (File)
Nov 27 2024, 5:23 AM

Details

Summary

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.

Test Plan
  • 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

Repository
rP Phabricator
Branch
phurlaccesscontroller
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 8547
Build 9864: arc lint + arc unit

Event Timeline

lpriestley retitled this revision from to Ref T8989, Phurl "Visit URL" link should route through a separate controller..
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.
joshuaspence added inline comments.
src/applications/phurl/controller/PhabricatorPhurlURLAccessController.php
17

Hmm, why is CAN_EDIT necessary?

lpriestley edited edge metadata.

Phurl shortened URL should be available to anyone who can view, even if they can't edit.

epriestley edited edge metadata.
epriestley added inline comments.
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.

This revision is now accepted and ready to land.Nov 2 2015, 7:49 PM
src/applications/phurl/controller/PhabricatorPhurlURLAccessController.php
17

Yeah, I supposed you want to open it to anyone who can view only.

src/applications/phurl/controller/PhabricatorPhurlURLViewController.php
112–113

Also, this should be maybe $can_view instead?

lpriestley edited edge metadata.

Updating the view/edit capabilities required for viewing shortened url, and updating how we're locating a protocol in the allowed protocol array.

Leaving setWorkflow to its default which is false.

Getting rid of unnecessary variable.

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.

This revision was automatically updated to reflect the committed changes.

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).