Summary: Ref T5861. Adds an option to opt out of all notification email. We'll still send you password resets, email verifications, etc.
Test Plan:
{F189484}
- Added unit tests.
- With preference set to different things, tried to send myself mail. Mail respected preferences.
- Sent password reset email, which got through the preference.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: rush898, epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10237
Summary:
Ref T5861. These two options are complex, rarely useful, and not directly related to controlling what mail you receive.
Move them to a separate panel to make way for more stuff on the preferences panel. We'll probably add an "HTML" option to this new panel eventually, too.
Test Plan:
{F189474}
- Used both panels.
- Tested with multiplexing off.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10236
Summary:
Fixes T5185. The fundamental issue is that this `excludePHIDs` property was not saved, so the logic went like this:
- Generate `excludePHIDs` correctly.
- Pass `excludePHIDs` through the stack.
- Perform some other computations correctly.
- Queue the mail for the daemons, throwing it away. {icon bomb}
- Daemons process mail with empty `excludePHIDs` list.
Store it in the persistent properties array instead.
Also remove the "override self mail" thing, since it's only used by `bin/mail send-test` and suffers from the same issue. I think it's too useless to fix, since even if you get caught by it, `bin/mail` makes it clear why the message was dropped.
Test Plan:
Notable:
- `exclude` present in properties
- Exclusion reason under RECIPIENTS header
{P1229}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10234
Summary:
Fixes T5456. We lost this logic in the transition to applicationtransactions.
When publishing a feed story, mark all of the object's projects as related, so the project filter in feed works.
Test Plan: Made a comment on a task associated with a project, saw the story in filtered feed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: timor, epriestley
Maniphest Tasks: T5456
Differential Revision: https://secure.phabricator.com/D10233
Summary:
Fixes T5233.
- The mail adapter API currently expects plain addresses (like `a@b.com`) in `addTos()`, and some adapters can not accept fancy verbose addresses (like `"name" <a@b.com>`).
- When we try to send error email, we pass the entire "From" header into the API. This is incorrect.
- Since it would be nice to make this just work in the future, fix it inside the API.
- Specifically, this is reached with: send email -> generates error -> we try to send you an email back -> we send it to your "From" -> some mailers choke on the fancy name if you have one.
Test Plan: Processed an errorneous email with a fancy "From", got a response error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5233
Differential Revision: https://secure.phabricator.com/D10232
Summary: Ref T5817. This just fixes the markup in emails, the overall behavior still isn't great. I don't want to spend to much time on Ponder until it ends up somewhere nearer the top of the priority queue.
Test Plan: Viewed feed stories and emails, no stray/clearly-broken HTML.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5817
Differential Revision: https://secure.phabricator.com/D10231
Summary: Fixes T5859. This doesn't change much, but makes the transaction record a little more accurate and activates stuff like `#hashtags` and `{F123}` causing policy associations.
Test Plan: Used `bin/mail receive-test` and mail receiver script to send bug mail, saw hashtags imply projects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5859
Differential Revision: https://secure.phabricator.com/D10229
Summary:
Fixes T5839. If a repository has been force pushed and garbage collected, we might have a ref cursor in the database which still points at the old commit (which no longer exists).
We'll then run a command like `git log <new hash> --not <old hash>` to figure out which commits are newly pushed, and this will bomb out because `<old hash>` is invalid.
Instead, validate all the `<old hash>` values before we try to make use of them.
Test Plan:
- Forced a repository into a bad state by mucking with the datbase, generating a reproducible failure similar to the one in T5839.
- Applied patch.
- `bin/repository update <callsign> --trace` filtered the bad commit and put the repository into the right state.
- Saw new commits recognized correctly.
- Ran `bin/repository update <callsign>` for a Mercurial and SVN repo as a sanity check.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5839
Differential Revision: https://secure.phabricator.com/D10226
Summary:
Fixes T5184. Fixes T5008. Three issues with stories/notifications about changing the status of tasks which block other tasks:
**Bad Feed Stories**
- Problem: Feed story rendering was confusing (T5184).
- Solution: fix it to provide context.
**Too Many Feed Stories**
- Problem: Feed gets a story for the original task's close ("a closed x"), and a story for each blocked task ("a closed x, a task blocking y").
- "Solution": Punt. These are redundant in the full feed but not in filtered feeds. Right solution is display-time aggregation. No users have really complained about this.
**Too Many Notifications**
- Problem: Users subscribed to both tasks get notified about the clsoe, and also about the unblocked task. These notifications are redundant.
- "Solution": Punt. This is easy to fix by silencing notifications for the sub-editor, but I'm worried it would be confusing. Users haven't complained. Display-time aggregation might be a better fix.
Test Plan: {F189463}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5008, T5184
Differential Revision: https://secure.phabricator.com/D10235
Summary: Ref T5819. Implements basic icon and color filtering for projects.
Test Plan: {F189350}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5819
Differential Revision: https://secure.phabricator.com/D10230
Summary:
Fixes T5855. Adds a `--graceful N` flag to `phd stop` and `phd restart`.
`phd` will send SIGINT, wait `N` seconds, SIGTERM, wait 15 seconds, and SIGKILL. By default, `N` is 15.
Test Plan:
- Ran `bin/phd debug ...` and used `^C` to interrupt daemons. Saw graceful shutdown behavior, and abrupt termination on multiple `^C`.
- Ran `bin/phd start`, `bin/phd stop` and `bin/phd restart` with `--graceful` set to various things, notably `0`. Saw graceful shutdowns on the CLI and in the web UI. With `0`, abrupt shutdowns.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5855
Differential Revision: https://secure.phabricator.com/D10228
Summary: Resolves T5836. This automatically releases artifacts when Harbormaster builds finish (either passing or failing). This allows Harbormaster to release the Drydock leases it has for hosts.
Test Plan: Tested it with a build plan that passes and fails; tested it with lots of builds running in parallel.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5836
Differential Revision: https://secure.phabricator.com/D10208
Summary: This allows timeouts to be specified on SSH connections that Drydock makes. Used in the EC2 allocator to poll for the SSH server starting.
Test Plan: Used in EC2 allocator diff.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10225
Summary:
Ref T1049. This keeps track of how long a build target takes to execute in Harbormaster and displays it in the build view page. I'm not sure whether "Started" is really that useful once the target has completed?
Also, I change the name of the time taken depending on whether or not the target has completed; if it's still in progress it's called "Elapsed" and if it's completed then it's "Duration". The primary reason for this is that "Duration" sounds like post tense, whereas "Elapsed" is current tense. I'm not sure whether this is okay or not?
Test Plan: Ran a Sleep build step and saw the target dates / times appear correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: talshiri, epriestley, Korvin
Maniphest Tasks: T5824, T1049
Differential Revision: https://secure.phabricator.com/D10174
Summary: To assist with {T5245}, I have added projects back into the lipsum maniphest generator with the edge infrastructure.
Test Plan: Run the lipsum script for PhabricatorManiphestTaskTestDataGenerator and make sure it generates project data.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10202
Summary: Fixes T5850. Also fixes some logic where the wrong preempting events could be attached during a bulk query.
Test Plan: Phrequent list now shows preemption-aware times.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5850
Differential Revision: https://secure.phabricator.com/D10223
Summary:
Fixes T5848.
- Disallow tracking negative time.
- Preserve note if there's an error with the time selection.
- Show start time and duration.
- Slightly better error messages.
Test Plan: Started and stopped time. Tried to select future/negative ranges.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5848
Differential Revision: https://secure.phabricator.com/D10218
Summary:
Via HackerOne. If a user adds an email address and typos it, entering `alinculne@gmailo.com`, and it happens to be a valid address which an evil user controls, the evil user can request a password reset and compromise the account.
This strains the imagination, but we can implement a better behavior cheaply.
- If an account has any verified addresses, only send to verified addresses.
- If an account has no verified addresses (e.g., is a new account), send to any address.
We've also received several reports about reset links not being destroyed as aggressively as researchers expect. While there's no specific scenario where this does any harm, revoke all outstanding reset tokens when a reset link is used to improve the signal/noise ratio of the reporting channel.
Test Plan:
- Tried to send a reset link to an unverified address on an account with a verified address (got new error).
- Tried to send a reset link to a verified adddress on an account with a verified address (got email).
- Tried to send a reset link to an invalid address (got old error).
- Tried to send a reset link to an unverified address on an account with only unverified addresses -- a new user (got email).
- Requested several reset links, used one, verified all the others were revoked.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10206
Summary: Ref T2787. This is very basic and just helps me know that the data is inserting correctly.
Test Plan: {F187765}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10205
Summary:
- Fixes T5851. Currently, if a commit has `Fixes T123`, we generate an email with just that before generating the commit email. Don't send/publish transactions about a commit before it imports (this is a tiny bit hacky, but well-contained and I don't think it causes any problems).
- Fixes T4864. Currently, we try to parse Differential information even if Differential is not installed. Instead, do this only if Differential is installed.
- Fixes T5771. Currently, if we can't figure out who the committer/author of a commit is, we don't publish a `Fixes T123` transaction. Instead, fall back to acting as "Diffusion" if we can't find a better actor. Most of this diff expands the role of application actors. The existing application actors (Herald and Harbormaster) seem to be working well.
Test Plan:
- Pushed a commit with `Fixes T123` and verified it did not generate email directly. (The task half of the transaction still does, correctly.)
- Uninstalled Differential and pushed a commit, got a clean import instead of an exception.
- Commented out author/committer PHIDs and pushed stuff, saw a "Diffusion" actor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5771, T4864, T5851
Differential Revision: https://secure.phabricator.com/D10221
Summary:
Fixes T5838.
- We currently try to use a `ConduitAPIMethod` object as a string.
- We then pass that string to the parent's `__construct()` method as `$message`.
Test Plan: Uninstalled Maniphest, then tried to execute `maniphest.createtask`. Got a useful exception message instead of an error during message construction.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5838
Differential Revision: https://secure.phabricator.com/D10211
Summary:
Fixes T5849. When a new file is created, we might have to actually write the data to a storage engine, or we might be able to just point at data which is already there.
Currently, these two paths handle `$params` with different code and mild behavioral differences. Instead, have them call the same code so they get the same behavior.
Test Plan:
- Uploaded the same file multiple times to home page.
- Uploaded the same file multiple times as profile picture.
- Generated files via Diffusion.
- All the files got the expected properties, whether they were reusing data or not.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5849
Differential Revision: https://secure.phabricator.com/D10216
Summary:
Ref T5685. Currently we just 403 on an invalid token, but we can be a little more helpful.
The issues here are:
- If we **do** redirect you on this page and something goes wrong, you might get stuck in a redirect loop.
- If we **don't** redirect you, copy/pasting the link to someone (or reloading the page) gives them a pretty confusing result, since the link doesn't work any more. Prior to this diff, they get a 403.
To mitigate this, do a little better than a bare 403: give them a link to auth and generate a new URI for the file.
If this is still confusing, the next best thing I can come up with is something like this:
- Put some modulous of the timestamp in the URI.
- If the current time is within 2 seconds of the generation time, show this dialog.
- Otherwise, redirect.
That seems like it would be okay, but I worry that "2" has to be small (so links you copy/paste -> chat -> click still work) and a small value means that a small amount of clock skew breaks things. We could use the database clock, but ehhh.
Other ideas:
- Put a hash of the remote IP in the URI, redirect if it doesn't match. Fails for companies behind a NAT gateway but should work in a lot of other cases.
- Just redirect always, there's no reason it should ever loop and browsers don't really do anything bad when there's a loop (they'll show an error after too many redirects).
I'm leaning toward letting this stabilize in the wild for a bit, then trying "always redirect".
Test Plan: {F188914}
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10215
Summary: Ref T5685. We've added a new `canCDN` flag to control whether or not files can be cached and delivered over a CDN. Show this flag in the UI.
Test Plan: Viewed several files, saw correct/expected UI values.
Reviewers: btrahan, 20after4
Reviewed By: 20after4
Subscribers: epriestley
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10213
Summary: See D10189. We should never hit this anymore, so clean it up.
Test Plan:
- Reloaded a board, saw everything stay where it was before the change.
- Added a new task to the project, saw it show up in backlog.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10200
Summary: Fixes T5829. This stuff is old and busted, but keep it working for now.
Test Plan: No more fatal when there are recently closed tasks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5829
Differential Revision: https://secure.phabricator.com/D10201
Summary:
Fixes T5677.
- Instead of using `sequence == 0` to mean "this is the backlog column", flag the column explicitly.
- Migrate existing sequence 0 columns to have the flag.
- Add the flag when initializing or copying a board.
- Remove special backlog logic when reordering columns.
Test Plan:
- Migrated columns, viewed some boards, they looked identical.
- Reordered the backlog column a bunch of times (first, last, middle, dragged other stuff around).
- Added tasks to a project, saw them show up in the reordered backlog.
- Initialized a new board and saw a backlog column show up.
- Copied an existing board and saw the backlog column come over.
- Tried to hide a backlog column.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5677
Differential Revision: https://secure.phabricator.com/D10189
Summary: This slipped through the datasource modernization stuff.
Test Plan: Used search UI.
Reviewers: rush898, btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10196
Summary:
Ref T5024, T4427, T5474, T5523. Instead of separate icons in the column header for "Create Task" and "Edit Column Settings", use a dropdown menu.
- T5024 will likely add a "View Standalone" option.
- T4427 needs header space to show a count.
- T5474 likely needs "Edit Triggers..." (this seems reasonable to separate from editing the name, etc.)
- T5523 likely adds "Move all tasks..." eventually.
Test Plan: {F187414}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5523, T5474, T5024, T4427
Differential Revision: https://secure.phabricator.com/D10190
Summary: Sets layout as flush when rendering diff table or timeline in a Dialog
Test Plan: Tested each
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10194
Summary: Fixes T5739. I only got D9857 half right: the new method names are correct, but the bodies needed to change too.
Test Plan: Signed a document as an anonymous user.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5739
Differential Revision: https://secure.phabricator.com/D10191
Summary:
Via the UI adding a mailinglist for CC works, but via
the API currently it shows:
>One or more PHIDs were invalid for ccPHIDS
This removes the user validation check for ccPHIDs.
(I left it in for other things like owner since that seems
still appropriate?)
Test Plan:
used arc locally to add a mailinglist to cc
```echo '{"id": 2, "ccPHIDs": ["PHID-MLST-ohduchbv4dfimk7opt3r"]}' | arc call-conduit maniphest.update```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10193
Summary:
Ref T4807. This is probably a complete fix, but I'd be surprised if there isn't a little cleanup I missed.
When users drag tasks on a "natural"-ordered workboard, leave things where they put them.
This isn't //too// bad since a lot of the existing work is completely reusable (e.g., we don't need any new JS).
Test Plan:
- Dragged a bunch of stuff around, it stayed where I put it after dropped and when reloaded.
- Dragged stuff across priorities, no zany priority changes (in "natural" mode).
- Created new tasks, they show up at the top.
- Tagged new tasks, they show up at the top of backlog.
- Swapped to "priority" mode and got sorting and the old priority-altering reordering.
- Added tasks in priority mode.
- Viewed task transactions for correctness/sanity.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: chad, epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10182
Summary:
Ref T4807. This is an alternative to D10179. The problem these diffs solve is that I want to be able to reorder a column's positions without having to load the actual objects, but that's difficutl because two positions may have the same sequence number (and I think it's good that we allow that, since it makes a bunch of other stuff way easier).
Instead of using the object ID (e.g., the task ID) to reorder positions with the same sequence, use the position itself. This is a little easier, is less ambiguous if columns eventually have several types of objects, and produces a better behavior when old objects are freshly added to a board. For example, if you tag `T300` with `#project`, this new rule will push it to the top of "Backlog" while the old rule might have buried it deep. I think this behavior is desirable and more "natural".
When creating a group of new rows, we do order the batch by ID, so a group of freshly-tagged objects float to the top togehter in ID order. This seems like the most natural rule, too.
Test Plan:
- Loaded some boards with implicit objects on them (freshly tagged tasks) and saw rows create.
- Verified new rows created in the right order.
- Dragged some tasks around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10180
Summary:
Ref T4807. This doesn't actually do anything yet, but adds a dropdown menu for choosing an ordering and gets all the UI working correctly.
This also fixes a bug where column hidden state wouldn't persist across filter changes.
(I won't land this until it does something, but the next diff will probably be a mess so this seemed like a clean place to sever things.)
Test Plan:
{F187114}
- Altered sort ordering.
- Altered hidden state and filters, verified all states persisted correctly.
- Added `phlog()` to edit/create and move controllers and verified they receive sort information.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: swisspol, chad, epriestley
Maniphest Tasks: T4807
Differential Revision: https://secure.phabricator.com/D10178
Summary:
CanCDN flag indicates that a file can be served + cached
via anonymous content distribution networks.
Once D10054 lands, any files that lack the CanCDN flag
will require a one-time-use token and headers will
prohibit cache to protect sensitive files from
unauthorized access.
This diff separates the CanCDN changes from the code that
enforces these restrictions in D10054 so that the changes
can be tested and refined independently.
Test Plan: Work in progress
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: rush898, qgil, epriestley, aklapper, Korvin
Maniphest Tasks: T5685
Differential Revision: https://secure.phabricator.com/D10166
Summary: Fixes T5705. This was just derp; instead of returning the duration of the first slice, return the duration of all the slices.
Test Plan: Added unit tests. Saw reasonable results in the UI.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5705
Differential Revision: https://secure.phabricator.com/D10184
Summary: Fixes T5423, "is newly created" herald rule fails on dry runs
Test Plan: Create herald "is newly created" rule, and do a dry run on an existing pholio mock, differential commit, or maniphest task. Should not return an exception.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5423
Differential Revision: https://secure.phabricator.com/D10187
Summary:
Ref T5245. This removes some hacks and activates two meaningful interactions:
- The "projects" field goes through shared code now.
- Mentioning projects in tasks using hashtags now tags them.
Test Plan:
- Viewed a task with projects.
- Viewed a task with no projects.
- Viewed a task with projects and board positions.
- Viewed a revision with projects.
- Made a `#hashtag` comment in Maniphest and got a project association.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D10177
Summary: Ref T1049. This allows build steps to create URI artifacts, which can be used to link to external builds and other resources.
Test Plan: Used a build step in an external library to test the creation of a URI artifact and verified it appeared correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D10173
Summary: Resolves T5814. Ref T1049. This changes "Stop" to "Pause" in the UI (internally it's still referred to as Stop).
Test Plan: Viewed builds and saw the intended wording.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T1049, T5814
Differential Revision: https://secure.phabricator.com/D10172
Summary: Use cutlery icon for hilarity. Ref T5768.
Test Plan: made something with remarkup in it, used 'view raw' and saw the remarkup raw in a nice little dialogue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5768
Differential Revision: https://secure.phabricator.com/D10183
Summary: Fixes T5478. For "personal" installs use the person icon; for global use the global icon. For both providing explanatory tooltip text about what's going on. This will need to be updated if / when we start installing dashboards to other applications. Also, this query isn't 100% optimized but the major part *is* so I think its okay.
Test Plan: Installed a dashboard for personal use and verified correct icon / text showed up. Did the same for global installed dashboard...!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5478
Differential Revision: https://secure.phabricator.com/D10181
