Page MenuHomePhabricator

Detail link in badge email is very, very, wrong
Closed, ResolvedPublic

Description

This /badge/ link should be /badges/.

Screen Shot 2015-08-20 at 4.44.35 AM.png (171×383 px, 22 KB)

Revisions and Commits

Event Timeline

epriestley raised the priority of this task from to Low.
epriestley updated the task description. (Show Details)
epriestley added a project: Badges.
epriestley added a subscriber: epriestley.

I've seen a bunch of cases where dead URLs have persisted for a while before being caught... Have you given any thought to improving the way that we handle URLs? I feel like there should be some getUrl() method for most (all?) objects.

getApplicationURI is available in most (all?) applications I believe

From memory, that just gets the /badges/ part of the URL? I'm talking about this (I'm on the train so excuse the pseudo-code):

$badge = id(new PhabricatorBadge())->loadOneWhere('id = 7');
echo $badge->getViewUrl();
// `/badges/view/7/`

getApplicationURI() only solves this for the first part of the path (although that happens to be the issue here), and isn't available inside the Editor where mail is being constructed. One thing we could do is throw if URIs passed to getApplicationURI() don't route when in development mode, although that doesn't feel amazing.

I haven't managed to come up with a very good solution to this problem, architecturally. The current approach is definitely not ideal: URIs get defined in Application classes, and then copied in a bunch of Controllers, sometimes in other places like Editors and PHIDTypes and so on. Lots of code duplication leading to bad/stale URIs.

Adding getURI() to objects decreases the code duplication somewhat, but it also makes the codebase more inconsistent (some URIs come from objects, some don't) and spreads authority for URIs farther across the stack.

I think the consistency issue is particularly messy because we don't really want getViewURI(), getEditURI(), getDisableURI(), etc., methods on everything, but unless we do that we're in a state where some URIs come from objects and others come from elsewhere.

Applications and Controllers are also really cumbersome and often not available in places (like Editors) where we want to generate URIs.

chad renamed this task from Detail link in badge email is wrong to Detail link in badge email is very, very, wrong.Aug 20 2015, 9:49 PM

I've historically thought about this mostly from the point of view of having the $object reach up to the $application and get URIs from it -- since the Application defines the routes -- but maybe that's not really the right way to tackle it, especially since it's hard to generate URIs from routes but relatively easy to generate routes from URIs.

Maybe the Application should just be saying "among other ad-hoc routes, I also route these objects", and all the routing lives on them: the objects get to be authorities for their routes. They'd implement some PhabricatorRoutableInterface and define the actions they accept (View, Edit, other custom actions), and then emit regexps for the routing table and URIs through some method on the Interface. Without traits this is probably either a lot of boilerplate per object or some kind of router helper class thing.

This still feels messy and like a lot of getURI() operations would amount to transforming the string /x/y/z/ to some structured representation of almost exactly the same data, but maybe it could be made clean-ish. I'm not hugely optimistic about being able to build something that feels good here, but it doesn't necessarily seem like a total dead end either.

Maybe I'll take a shot at this before T9132 starts rolling out since that will probably involve touching every /edit/ route in every application, much like we previously replaced every /query/ route for ApplicationSearch.