Summary:
Ref T10748. These:
- Look nice.
- Hint at panel contents / effects.
- Hint which panels have been customized.
- Allow panels with issues or errors to be highlighted with an alert/attention icon.
Test Plan: {F1256156}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15836
Summary: Ref T10748. This copies existing code in the `CreateController` which will eventually be removed.
Test Plan:
- Created a new repository with the EditPro workflow.
- Saw it come up into the cluster properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15835
Summary:
Fixes T10912. When you drag tasks within a milestone, we currently apply an overbroad, API-focused rule and add the parent board's project. This logic was added fairly recently, as part of T6027, to improve the behavior of API-originated moves.
Later on, this causes the task to toggle in and out of the parent project on every alternate drag.
This logic is also partially duplicated in the `MoveController`.
- Add test coverage for this interaction.
- Fix the logic so it accounts for subproject / milestone columns correctly.
- Put all of the logic into the TransactionEditor, so the API gets the exact same rules.
Test Plan:
- Added a failing test and made it pass.
- Dragged tasks around within a milestone column:
- Before patch: they got bogus project swaps on every other move.
- After patch: projects didn't change (correct).
- Dragged tasks around between normal and milestone columns.
- Before patch: worked properly.
- After patch: still works properly.
Here's what the bad changes look like, the task is swapping projects with every other move:
{F1255957}
The "every other" is because the logic was trying to do this:
- Add both the parent and milestone project.
- Whichever one exists already gets dropped from the change list because it would have no effect.
- The other one then applies.
- In applying, it forces removal of the first one.
- Then this process repeats in the other direction the next time through.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10912
Differential Revision: https://secure.phabricator.com/D15834
Summary:
Ref T10748. This allows an EditEngine form to be broken up into pages.
This is less powerful than `PHUIPagedFormView`, because the pages are not sequential / stateful. Each form saves immediately once it's submitted, and can not take you to a new form or back/forward in a series of forms.
For example, you can't create a workflow where the user fills out 5 pages of information before we create an object, like the current repository workflow does.
However, the only place we've ever wanted to do this is repositories and it's fairly bad there, so I feel reasonably confident we aren't going to miss this in the future.
(We do "choose a type of service/repository/rule -> fill out one page of info" fairly often, but can do this without the full-power paging stuff.)
Test Plan:
- Created a repository usin the new Manage UI, filling out only a handful of fields.
- Edited a repository using the new Manage UI.
- All forms are now EditEngine forms offering paged views of the big huge underlying form:
{F1254371}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15832
Summary:
Ref T10748. Ref T10366. Allows users to set credential for new URIs.
- Ref T7221. Our handling of the "git://" protocol is currently incorrect. This protocol is not authenticated, but is considered an SSH protocol. In the new UI, it is considered an anonymous/unauthenticated protocol instead.
- Ref T10241. This fixes the `PassphraseCredentialControl` so it doesn't silently edit the value if the current value is not visible to you and/or not valid.
Test Plan:
Performed a whole lot of credential edits, removals, and adjustments. I'll give this additional vetting before cutting over to it.
{F1253207}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7221, T10241, T10366, T10748
Differential Revision: https://secure.phabricator.com/D15829
Summary: Fixes T10906, Fixes T10820. Adds new icons, grey-er colors for previous states. Also, I think fixed a few bugs?
Test Plan: Fake each state, verify icon is as intended.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10820, T10906
Differential Revision: https://secure.phabricator.com/D15830
Summary: This is misspelled.
Test Plan: Consulted a dictionary.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15827
Summary:
Ref T10748.
- Allow users to add new URIs by clicking a button instead of knowing a secret URI.
- Validate that URIs are actually valid URIs.
- Add enable/disable action and strings.
Test Plan:
- Created a new URI.
- Tried to create a nonsense URI, created a good URI.
- Enabled/disabled a URI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15825
Summary:
Fixes T10905. This reverts D15823, which didn't work well for tasks with very long titles (the title would break as a block element).
This is slightly more magic but works with long titles.
Test Plan: Did everything from D15823, but also with long titles. Triple-click, wrapping, and mobile/device worked in Safari, Firefox and Chrome.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10905
Differential Revision: https://secure.phabricator.com/D15824
Summary:
Fixes T10905. In Firefox, triple clicking the new headers doesn't select the entire line, so you can't easily copy/paste an entire task title or revision name. It works fine in Safari/Chrome.
This seems to fix that without breaking anything.
Test Plan:
- Viewed headers in Safari, Firefox, Chrome.
- Triple-clicked headers in Safari, Firefox, Chrome.
- Viewed tablet/device layouts in Safari, Firefox, Chrome.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10905
Differential Revision: https://secure.phabricator.com/D15823
Summary: Ref T10748. Adds a "uris" attachment with URI information.
Test Plan: Queried URI information via Conduit, saw reasonable looking information.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15822
Summary: Ref T10748. Brings the rest of the transactions to EditEngine, supports creating via API.
Test Plan:
- Created a URI via API.
- Created a URI via web.
- Tried to apply sneaky transactions, got rejected with good error messages. <_< >_>
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15821
Summary: Ref T10748. Ref T10366. This documents how everything is planned to work shortly.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler, scode
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15817
Summary:
Ref T10748.
- New View page for repository URIs.
- Make display and I/O behavior (observe, mirror, read, read/write) editable.
- Add a bunch of checks to prevent you from completely screwing up a repository by making it writable from a bunch of differnet sources.
Test Plan:
{F1249866}
{F1249867}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15816
Summary:
Ref T10748. Ref T10366. This adds a new EditEngine, EditController, Editor, Query, and Transaction for RepositoryURIs.
None of these really do anything helpful yet, and these URIs are still unused in the actual application.
Test Plan: {F1249794}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15815
Summary:
Ref T10748. Allow the new EditEngine workflow to create repositories by giving the user a modal repository type choice upfront.
(The rest of this flow is still confusing/weird, though.)
Test Plan:
- Created a new repository.
{F1249626}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15813
Summary: Ref T10748. This brings the "Actions" items (publish/notify + autoclose enabled) into the new UI.
Test Plan:
- Edited this stuff via EditEngine and Conduit.
- Viewed via new Manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15811
Summary: Ref T10748. Makes a "Branches" panel, enables these transactions in the EditEngine.
Test Plan:
- Edited via EditEngine + Conduit.
- Viewed via manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15809
Summary: Ref T10748. Port this, add EditEngine support, add some type validation to the transaction.
Test Plan:
- Edited via EditEngine.
- Edited via Conduit.
- Viewed via Management UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15808
Summary:
Ref T10843. There are actually two separate notions of cacheability here:
- Is this cacheable by the browser (e.g., should we emit "Expires: long in the future")?
- Is this cacheable locally (e.g., should we stick it in APC, or just read it off disk every time)?
These got a little mixed up by D15775, so we aren't currently emitting proper "Expires" headers on font files and a few other resource types.
Straighten this out so that we "Expires" these unusual resources correctly.
Test Plan: Verified that `.woff` files get a proper "Expires" header now, not just CSS/JS.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10843
Differential Revision: https://secure.phabricator.com/D15807
Summary: Ref T10748. Brings this forward in the UI and EditEngine.
Test Plan:
- Edited via Conduit.
- Viewed via Manage UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15805
Summary: Ref T10748. Ports this UI and exposes it on the EditEngine.
Test Plan:
- Edited via EditEngine.
- Viewed new manage UI.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15804
Summary:
We're sometimes getting duplicate notifications right now. I think this is because multiple windows are racing and becoming leaders.
Clean this up a little:
- Fix the `timeoout` typo.
- Only try to usurp once.
- Use different usurp and expire delays, so we don't fire them at the exact same time.
Not sure if this'll work or not but it should theoretically be a little cleaner.
Test Plan:
- Quit Safari, reopened Safari, still saw a fast reconnect to the notification server (this is the goal of usurping).
- Did normal notification stuff like opening a chat in two windows, got notifications.
- Hard to reproduce the race for sure, but this at least fixes the outright `timeoout` bug.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15806
Summary:
Fixes T10778. This is a result of T10262: when we save a form configuration and adjust the policy, we try to scramble attached file secrets.
There aren't going to be any attached files, but there's also no edge table, so we fail.
We could skip this code, but we'll likely need an edge table here sooner or later so it's probably simpler in the long run to just add an empty one.
Test Plan:
- Ran `bin/storage upgrade`, got a clean bill of health.
- Saved a form configuration after making a policy edit, no more `edge` exception.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10778
Differential Revision: https://secure.phabricator.com/D15803
Summary: Ref T4292. This provides at least some sort of hint about how to set up cluster repositories.
Test Plan:
- Read documentation.
- Ran `bin/repository clusterize` to add + remove clusters.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15798
Summary:
Ref T10866. Fixes T10386. This attempts to make it a little more plausible to follow these directions:
- Use simpler language in general.
- Remove language suggesting that HTTP requires no additional configuration.
- Suggest using a load balancer or an ugly port number instead of swapping SSH to a different port.
- Be more granular about `sudo` setup.
- Organize better?
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10386, T10866
Differential Revision: https://secure.phabricator.com/D15796
Summary:
Fixes T10876. Currently, we can end up with a setup warning banner sticking on each web device, since the state is stored in local cache.
Instead:
- When we actually run the setup checks, save the current state in the database.
- Before we show a cached banner, make sure the database still says the checks are a problem.
This could lead to some inconsistencies if setup checks legitimately pass on some hosts but not on others. For example, if you have `git` installed on one machine but not on another, we may raise a setup warning ("No Git Binary!") about it on one host only.
For now, assume users have their operational environments in some sort of reasonable shape and can install the same stuff everywhere. In the future, we could split the issues into "global" and "per-host" issues if we run into problems with this.
Test Plan:
This is somewhat tricky to test locally since you really need multiple webservers to test it properly, but I:
- Created some setup issues, saw banner.
- Ignored/cleared them, saw banner go away.
- Verified database cache writes were occurring properly.
Then I sort of faked it like this:
- Created a setup issue.
- Manually set the database cache value to `[]` ("no issues").
- Reloaded page.
- No more banner.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10876
Differential Revision: https://secure.phabricator.com/D15802
Summary: Ref T10748. Brings this over and adds EditEngine support for it.
Test Plan: Viewed and edited staging area information.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15801
Summary: Ref T10748. This merges "Storage" and "Cluster" into a single UI which combines the information of both.
Test Plan: {F1246882}
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15800
Summary:
Ref T10849. This enforces a global 30-second per-query time limit for anything not coming from the CLI.
If we run into another issue with MySQL hanging in the future, this should prevent it from being nearly as bad as it was.
Test Plan:
- Set value to 0, verified the UI threw an exception immediately.
- Set value back to 30, browsed around a bunch of pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10849
Differential Revision: https://secure.phabricator.com/D15799
Summary:
Ref T4292. This is a required step in configuring a cluster: document and explain it.
Previously `bin/almanac register` could //also// add and trust keys. I've removed this capability since I think it's needless and complicated. If there's some real use for it eventually, we could add a `bin/almanac add-key` or whatever. The workflow is simpler and has better guard rails that point you in the correct direction now.
Test Plan:
- Read documentation.
- Ran `bin/almanac` with various good/bad flags.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15795
Summary: This gets over-escaped instead of bolded right now, but I only ever hit it when exporting/importing and never both cleaning it up.
Test Plan: Ran `bin/repository move-paths`, saw bolded "Move" instead of ANSI escape sequences.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15797
Summary:
Ref T10860. This allows us to recover if the connection to the database is lost during a push.
If we lose the connection to the master database during a push, we would previously freeze the repository. This is very safe, but not very operator-friendly since you have to go manually unfreeze it.
We don't need to be quite this aggressive about freezing things. The repository state is still consistent after we've "upgraded" the lock by setting `isWriting = 1`, so we're actually fine even if we lost the global lock.
Instead of just freezing the repository immediately, sit there in a loop waiting for the master to come back up for a few minutes. If it recovers, we can release the lock and everything will be OK again.
Basically, the changes are:
- If we can't release the lock at first, sit in a loop trying really hard to release it for a while.
- Add a unique lock identifier so we can be certain we're only releasing //our// lock no matter what else is going on.
- Do the version reads on the same connection holding the lock, so we can be sure we haven't lost the lock before we do that read.
Test Plan:
- Added a `sleep(10)` after accepting the write but before releasing the lock so I could run `mysqld stop` and force this issue to occur.
- Pushed like this:
```
$ echo D >> record && git commit -am D && git push
[master 707ecc3] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 254 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
BEGIN SLEEP
```
- Here, I stopped `mysqld` from the CLI in another terminal window.
```
END SLEEP
# CRITICAL. Failed to release cluster write lock!
# The connection to the master database was lost while receiving the write.
# This process will spend 300 more second(s) attempting to recover, then give up.
```
- Here, I started `mysqld` again.
```
# RECOVERED. Link to master database was restored.
# Released cluster write lock.
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
2cbf87c..707ecc3 master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15792
Summary:
Ref T10860. At least in Git over SSH, we can freely echo a bunch of stuff to stderr and Git will print it to the console, so we can tell users what's going on.
This should make debugging, etc., easier. We could tone this down a little bit once things are more stable if it's a little too chatty.
Test Plan:
```
$ echo D >> record && git commit -am D && git push
[master ca5efff] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 256 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
8616189..ca5efff master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15791
Summary: Ref T10860. This doesn't change anything, it just separates all this stuff out of `PhabricatorRepository` since I'm planning to add a bit more state to it and it's already pretty big and fairly separable.
Test Plan: Pulled, pushed, browsed Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15790
Summary:
Fixes T10865.
- Mock descriptions did not markup.
- Image descriptions did not get a proper container `<div />`.
Test Plan:
- Created a mock with remarkup in the mock description and in an image description.
- Viewed mock detail.
- Saw list styles render properly in both mock description and image description.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10865
Differential Revision: https://secure.phabricator.com/D15793
Summary: Fixes T9560. We suggest a root-owned location, but users who choose their own location instead can run into trouble.
Test Plan:
- Changed parent directory to have an non-root owner, verified that `ssh` no longer worked.
- Changed parent directory back to a root owner, verified `ssh` worked again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9560
Differential Revision: https://secure.phabricator.com/D15794
Summary: Fixes T10863. See that task for discussion.
Test Plan:
- Configured `aphlict` with no "logs".
- Started `aphlict`.
- Before change: exception.
- After change: worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10863
Differential Revision: https://secure.phabricator.com/D15788
Summary: Fixes T10857. This documentation did not accurately reflect proper configuration: in the Aphlict config, SSL is inferred from the presence of `ssl.*` configuration.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10857
Differential Revision: https://secure.phabricator.com/D15787
Summary:
Ref T4292. Sometimes, we may not have a working copy for a repository. The easiest way to get into this condition is to deactivate a repository.
We could try to clone + fetch in this case, but that's kind of complex, and there's an easy command that administrators can run manually. For now, just tell them to do that.
This affects the inactive repositories on `secure`, like rGITCOINS.
Test Plan: Removed working copy, got message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15786
Summary:
Ref T7789. Currently, we use different viewers if you have `security.alternate-file-domain` configured vs if you do not.
This is largely residual from the days of one-time-tokens, and can cause messy configuration-dependent bugs like the one in T7789#172057.
Instead, always use the omnipotent viewer. Knowledge of the secret key alone is sufficient to access a file.
Test Plan:
- Disabled `security.alternate-file-domain`.
- Reproduced an issue similar to the one described on T7789.
- Applied change.
- Clean LFS interaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15784
Summary:
Ref T4292. When the daemons make a query for repository information, we need to make sure the working copy on disk is up to date before we serve the response, since we might not have the inforamtion we need to respond otherwise.
We do this automatically for almost all Diffusion methods, but this particular method is a little unusual and does not get this check for free. Add this check.
Test Plan:
- Made this code throw.
- Ran `bin/repository reparse --message ...`, saw the code get hit.
- Ran `bin/repository lookup-user ...`, saw this code get hit.
- Made this code not throw.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15783
Summary: I've possibly seen a couple of `aphlict` processes exit under suspicious circumstances (maybe?). Make sure any PHP errors get captured into the log.
Test Plan:
- Added an exception after forking.
- Before change: vanished into thin air.
- After change: visible in the log.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15782
Summary:
Fixes T10843. In a multi-server setup, we can do this:
- Two servers, A and B.
- You push an update.
- A gets pushed first.
- After A has been pushed, but before B has been pushed, a user loads a page from A.
- It generates resource URIs like `/stuff/new/package.css`.
- Those requests hit B.
- B doesn't have the new resources yet.
- It responds with old resources.
- Your CDN caches things. You now have a poisoned CDN: old data is saved in a new URL.
To try to avoid this with as little work as possible and generally make it hard to get wrong, check the URL hash against the hash we would generate.
If they don't match, serve our best guess at the resource, but don't cache it. This should make things mostly keep working during the push, but prevent caches from becoming poisoned, and everyone should get a working version of everything after the push finishes.
Test Plan:
- `curl`'d a resource, got a cacheable one.
- Changed the hash a little, `curl`'d again. This time: valid resource, but not cacheable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10843
Differential Revision: https://secure.phabricator.com/D15775
Summary: Ref T4292. This adds a new high-level overview panel.
Test Plan: {F1238854}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15772
Summary: Fixes T8897
Test Plan: Open any list view of Calendar events, every event should only show "Attending: ..." with users who are attending event.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8897
Differential Revision: https://secure.phabricator.com/D15779
Summary: This still wasn't quite right -- a link like `[[ Porcupine Facts ]]` with a space would not lookup correctly, and would render as `porcupine_facts`.
Test Plan: Verified that `[[ Porcupine Facts ]]` now works correctly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15780
Summary: Some of these had the wrong `@group` header.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15778
Summary: Fixes T10845.
Test Plan: Verified that `[[ quack ]]` and `[[ QUACK ]]` both work. Previously, the link had to exactly match the capitalization of the target.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10845
Differential Revision: https://secure.phabricator.com/D15777
