This /badge/ link should be /badges/.
Description
Revisions and Commits
rP Phabricator | |||
D13950 | rP89336197e380 Correct link in Badges email |
Related Objects
- Mentioned In
- D16597: Remarkup rule to embed images
- Mentioned Here
- T9132: Build an ApplicationEditor abstraction
Event Timeline
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.
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.
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.