Summary:
The linter's behavior was changed in https://github.com/jshint/jshint/issues/3444: "warnings" are no longer counted for `maxerr`.
Updating the test to match...
Test Plan: `arc unit src/lint/linter/__tests__/ArcanistJSHintLinterTestCase.php` with a recent-ish (2.13.6) jshint.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25355
Summary:
Per https://www.php.net/manual/en/function.mb-convert-encoding.php, as of PHP 8.0.0, a ValueError is thrown if the value of `to_encoding` or `from_encoding` is an invalid encoding but a ValueError is not suppressed by the stfu operator ("@").
Origin of the function:
https://secure.phabricator.com/rPHU72ad8fd0f05b0d84f7d8efd7db62ad0b3ba4431f
Premising that Arcanist elevates warnings to exception, now we just try-catch.
Closes T15423
Test Plan:
On `/diffusion/edit/1/page/encoding/`,
* enter a valid encoding, such as "7bit", successfully changed encoding
* enter a valid encoding with random capitalization, such as "7biT", successfully changed encoding
* enter a valid alias encoding, such as "ISO-10646-UCS-2", successfully changed encoding
* enter a valid alias encoding with random capitalization, such as "isO-10646-uCS-2", successfully changed encoding
* enter an invalid encoding, such as "whatever", get error message "Repository encoding "whatever" is not valid: String conversion from encoding 'UTF-8' to encoding 'whatever' failed: mb_convert_encoding(): Argument #2 ($to_encoding) must be a valid encoding, "whatever" given"
In any case, no exception is shown anymore.
Reviewers: O1 Blessed Committers, valerio.bozzolan, speck
Reviewed By: O1 Blessed Committers, valerio.bozzolan, speck
Subscribers: 0, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15423
Differential Revision: https://we.phorge.it/D25249
Summary:
Note that booleans are scalars. Full stop.
| is_scalar($v) | Result |
|------------------|--------|
| `"foo"` | true |
| `""` | true |
| `null` | true |
| `0` | true |
| `0.5` | true |
| `true` | true |
| `false` | true |
| `new stdclass()` | false |
| `array()` | false |
Note that phutil_nonempty_scalar() was designed just to tell
whenever a scalar is "empty" or not. So it must not explode
when receiving a valid scalar.
So the question is not whenever a boolean is a scalar or not,
but whenever is empty or not. But also this is a clear fact:
| `$v` | `is_scalar($v)` | `!is_empty($v)` | `if(strlen($v))`|
|---------|-----------------|-----------------|-----------------|
| `true` | `true` | `true` | `true` |
| `false` | `true` | `false` | `false` |
In short, now the function does not explode anymore with bool
values. Instead, it says whenever are empty or not.
In bold the exact changes:
| Value |`phutil_nonempty_scalar($v)`|
|-------------------|----------------------------|
| `"foo"` | true |
| `""` | false |
| `null` | false |
| `0` | true |
| `0.5` | true |
|`obj` with tostring| true |
|`obj` withno tostr.| Exception |
| `array()` | Exception |
| `true` | ~~Exception~~ **true** |
| `false` | ~~Exception~~ **false** |
Closes T15239
Test Plan:
- check if it makes sense to you
- check the few usages
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15239
Differential Revision: https://we.phorge.it/D25117
Summary: Ref T15064
Test Plan: Run `arc browse <filename>`, get a browser and no exception.
Reviewers: O1 Blessed Committers, valerio.bozzolan, chris
Reviewed By: O1 Blessed Committers, chris
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15064
Differential Revision: https://we.phorge.it/D25309
Summary:
When arcanist connects to a phorge site which uses an SSL certificate signed by a local CA, then the client needs to have a resources/ssl/custom.pem file as per resources/ssl/README
As this is client specific it should be in the .gitignore file.
Also update resources/ssl/README to replace [Pp]habricator with [Pp]horge.
Test Plan:
```
touch resources/ssl/custom.pem
git status
```
The 'git status' should show no changes.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25304
Summary:
Fix the following error in ArcanistUnitConsoleRenderer.php under PHP 8.1:
```
strlen(): Passing null to parameter #1 ($string) of type string is deprecated
```
Stack trace:
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418, custom=4)
#0 PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/unit/renderer/ArcanistUnitConsoleRenderer.php:15]
#1 ArcanistUnitConsoleRenderer::renderUnitResult(ArcanistUnitTestResult) called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:190]
#2 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:427]
```
Ref T15187
Test Plan: arc unit
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15187
Differential Revision: https://we.phorge.it/D25300
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_stringlike()` as a replacement for string-alike variables.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_stringlike() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes first half of T15331
Test Plan: Applied these two changes (one in Arcanist, one in Phorge). Project with empty Description field was created and `/project/view/projectid/` rendered in web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan, speck
Reviewed By: O1 Blessed Committers, valerio.bozzolan, speck
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15331
Differential Revision: https://we.phorge.it/D25176
Summary:
Passing null as input strings to `substr()` and `preg_match()` is deprecated in PHP 8.
Thus do not call `substr()` when input is `null` and pass an empty string instead of `null` to `preg_match()`. (Not calling `preg_match()` at all here would lead to `Exception: Lexical error on line 1. Unrecognized text. ^`).
Closes T15346
Test Plan: After applying these three changes and following the steps in T15346, tab on the dashboard displays "This tab panel does not have any tabs yet." as expected instead of the previous RuntimeException.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15346
Differential Revision: https://we.phorge.it/D25250
Summary:
This change fixes the command `arc look remotes` for PHP 8.1.
Without this change, the null value bubbles up to PhutilUTF8StringTruncator, reaching a strlen().
This control probably does not need to be done at this low level inside PhutilUTF8StringTruncator,
but it is right to be at this high level from the caller in ArcanistRefView.
Closes T15368
Test Plan:
- run "arc look remotes"
- still works in "old PHP" like 7.4
- start to work in recent PHP 8.1+
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15368
Differential Revision: https://we.phorge.it/D25206
Summary:
When there is no active branch name, arc diff currently fails under PHP8 when we try to strlen(null).
This change is also credited to Evan from upstream Phabricator that applied the same change:
https://secure.phabricator.com/rARCc39ab20eb3717a15aed2467842bd77d9addce96a
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15412
Test Plan: Under PHP 8.1: ran git checkout <hash of head>, then arc diff to generate this revision.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15412
Differential Revision: https://we.phorge.it/D25237
Summary:
`array_fuse` in Arcanist is a wrapper for calling `array_combine($list, $list)`.
The latter doesn't accept passing `null` in PHP 8.2.
Going to `/conduit/method/project.create/`, entering a `name` but nothing as `members` (so we pass `null`), and calling this method, an exception is thrown.
Thus make `array_fuse` accept null and return an empty list in such cases.
Closes T15393
Test Plan: Applied this change; afterwards "Method Call Result" page at `/api/project.create` correctly displayed in the web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15393
Differential Revision: https://we.phorge.it/D25228
Summary:
Mercurial 6.4 was recently released and showing up in package managers. With
the update to 6.4 using `arc land` would result in an exception indicating that
`expandpath` function does not exist.
The `ui.expandpath` function was deprecated in 5.8 and now removed in 6.4. The
functionality has been moved to `utils.urlutil.get_` functions (they are split
between getting pull, push, and clone paths).
This updates the script to try `utils.urlutil.get_clone_path` function if the
`ui.expandpath` function is not present.
Imported from:
https://secure.phabricator.com/rARC0fc22183e796fb8ac2e3a0a3f3f37aa964c6d7fa
Test Plan:
I updated my latest mercurial install to 6.4 and verified with `hg --version`.
I created a diff in a mercurial repo and used `arc land` to successfully land
the revision without any exceptions.
Closes T15288
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15288
Differential Revision: https://we.phorge.it/D25143
Summary:
For some reason it may happen that a specific command line argument receives a null argument
from PHP, instead of just an empty string.
Nowadays, this null value probably breaks almost whatever GNU/Linux or FreeBSD or Microsoft Windows
etc. implementations since everyone expect to receive a string.
This used to work in the past since functions like strpos() or strlen() accepted null, but not
anymore. This generate a deprecation warning since PHP 8.1, that is elevated as exception from
Phabricator/Phorge and breaking features.
Without getting into implementation logics (which doesn't make sense to fix all of them) the
calling function should just be kind. So we normalize nonsense null values to an empty string.
Note: this was the expected behavior prior to PHP 8.1.
Now we do that normalization explicitly, in this early point.
After this fix, also T15368 should probably be fixed.
Closes T15367
Test Plan:
- run "arc patch <something valid>"
- to you it must continue to work
- (to @ton it starts working right now)
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno, ton
Maniphest Tasks: T15367
Differential Revision: https://we.phorge.it/D25205
Summary:
After PHP 8.1 the function `rawurlencode()` does not accept anymore the `null` value.
Thus return an empty string when the input parameter is null instead of passing the input parameter to `rawurlencode()`.
Closes T15263
Test Plan:
Applied this change on top of D25144, D25145, D25146, D25147, D25151,
D25152, D25153 and D25163 and already existing Workboard located at
`/project/view/1/` finally rendered in web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15263
Differential Revision: https://we.phorge.it/D25164
Summary:
Passing `null` to the `$string` parameter of `mb_convert_case()` is deprecated in PHP 8.1.
This is one of the exceptions which block rendering the "Browse Projects" overlay dialog.
Closes part of T15335
Test Plan: Applied this change in Arcanist (plus the four changes in D25179 in Phorge) and the `Browse Projects` overlay dialog finally rendered in web browser and listed existing projects.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15335
Differential Revision: https://we.phorge.it/D25180
Summary:
Entering a malformed string (like `"pHoRgE rOcKs!!"`) into the "Raw Diff" field, Arcanist's `tryMatchHeader()` function is first called in `$ok = $this->tryMatchHeader($patterns, $line, $match)` with a non-null `$line` value (the first line entered in the "Raw Diff" field) being passed.
Afterwards, `tryMatchHeader()` is called for a second time after assigning `$line = $this->nextLineThatLooksLikeDiffStart()`.
This time `$line` is null and a RuntimeException is thrown, as `tryMatchHeader()` calls `preg_match()` which does not accept passing null as the $subject string parameter in PHP 8.1.
Thus add a `phutil_nonempty_string()` check if the `$subject` parameter (in this case, `$line`) is a non-empty string.
Arcanist's `tryMatchHeader()` function is not called outside of the file in which it is defined.
Thus catch the exception in the second call to `tryMatchHeader()` and not in the code of the `tryMatchHeader()` function itself.
Closes T15338
Test Plan: After adding the additional check, `/differential/diff/create/` showed the expected `Diff Parse Exception` instead.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15338
Differential Revision: https://we.phorge.it/D25183
Summary:
Conduit responds to requests with either `ERR-INVALID-SESSION` or `ERR-INVALID-AUTH` if the request wasn't sufficiently authenticated. Arcanist's `patch` workflow can automatically attempt to recover from situations in which Conduit responds to unauthenticated requests with `ERR-INVALID-SESSION` (by resending an authenticated version of the request), but not `ERR-INVALID-AUTH` - recover from `ERR-INVALID-AUTH` in the same way.
Closes T15333
Test Plan: The company I work for has been running a local clone of Arcanist containing this change in production for over 18 months now with no problems.
Reviewers: #blessed_committers, O1 Blessed Committers, valerio.bozzolan, avivey
Reviewed By: #blessed_committers, O1 Blessed Committers, valerio.bozzolan, avivey
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15333
Differential Revision: https://we.phorge.it/D25178
Summary:
This change fixes a RuntimeException for passing null to strlen() when setting up the DB host parameter
Closes T15260
Test Plan: I was able to run "./bin/config set mysql.host "localhost"" successfully
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: Ekubischta, goddenrich, Dylsss, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15260
Differential Revision: https://we.phorge.it/D25129
Summary:
In PHP 8.1, the strtolower() function no longer accepts NULL.
Without this change, getMatchSeverity() exits early if there is no severity found.
Closes T15257
Ref T15190
Test Plan: I ran this with an arc linter on a private repository which doesn't have a regex to match severity
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15257, T15190
Differential Revision: https://we.phorge.it/D25126
Summary:
This change fixes 'arc patch' in some circumstances.
Closes T15254
Test Plan: I was able to run "arc patch D25111" without issues
Reviewers: O1 Blessed Committers, Matthew
Reviewed By: O1 Blessed Committers, Matthew
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15187, T15254
Differential Revision: https://we.phorge.it/D25123
Summary:
Use the name "Phorge" as the defined platform.
Also prepare to rename the core library "phorge" rather then "phabricator" - see next diff.
T15006
Test Plan: Deployed change, tooltip for "Config" shows "Configure Phorge"
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew
Maniphest Tasks: T15006
Differential Revision: https://we.phorge.it/D25046
Summary:
Ref T13588. "arc-ls-markers" emits a "branch-state" marker so callers can identify which branch is active in the working copy.
This marker doesn't have an associated commit, so trying to generate a display name fails under stricter PHP 8.1 rules when we try to `substr(null, ...)`.
Don't attempt to generate a display name for markers with no commit hash.
Test Plan:
- Ran `arc branches` under PHP 8.1 in a Mercurial repository.
- Before: fatal.
- After: sensible output.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21825
Summary: Ref T13588. Additional PhutilURI fixes for PHP 8.1.
Test Plan: Ran "arc unit --everything" in Phabricator.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21822
Summary: Ref T13603. This is just a small piece of cleanup I've wanted to do for a while: different languages might have different list separators and repeating this implosion manually all over the place is a bit ugly even if the beahvior is never a function of translation language.
Test Plan: See next change.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21814
Summary:
Ref T13675. When a process daemonizes, it needs to close these actual file descriptors, so the calling context must provide them.
Elsewhere, modify the embedded "arc" to provide them. Then use them in place of reopening the streams.
Test Plan: Ran locally with embedded "arc", will deploy for production hangs.
Maniphest Tasks: T13675
Differential Revision: https://secure.phabricator.com/D21804
Summary:
Ref T13676. With increased PHP8.1 strictness around null, it seems reasonable to add some convenience parsing to "PhutilArgumentParser".
Add a helper function for parsing integers.
Test Plan:
See next change. Tried these arguments:
```
--count ''
--count asdf
--count 0
--count -3
--count 999999999999999999999999999999999999999999999234
--count 5
```
Got sensible parsing and appropriate feedback in all cases.
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21799
Summary: Ref T13676. Ref T13588. These properties on PhutilURI have flexible types, just wrap them.
Test Plan: Built a new Drydock working copy with an HTTPS URI under PHP 8.1, see T13676.
Maniphest Tasks: T13676, T13588
Differential Revision: https://secure.phabricator.com/D21798
Summary:
Ref T13675. Ref T13556. The "STDOUT" and "STDERR" constants are defined by the PHP CLI SAPI, in `cli_register_file_handles()`.
The "native arc" embedded PHP wrapper doesn't define these, and there's no real reason to define them, since they're just defined in terms of the PHP stream wrappers ("php://stdin", etc) anyway.
This patch isn't exhaustive (and a subsequent change should add lint, rejecting these magic constants) but is just trying to make native `arc` functional.
Test Plan: Created this revision with a standalone native `arc` binary.
Subscribers: cspeckmim
Maniphest Tasks: T13675, T13556
Differential Revision: https://secure.phabricator.com/D21794
Summary:
Ref T13658. The lint rule called "getStringLiteralValue()", which produces string literals for fewer nodes than "evalStatic()".
Switch to "evalStatic()", then fix new warnings.
Test Plan:
This test plan is non-exhaustive.
- Ran "arc lint --everything --output summary" to generate new warnings.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21776
Summary:
Ref T13658. Remove all product name literals from "pht()" strings, by replacing them with generic text where that feels reasonably natural, or "PlatformSymbols" calls elsewhere.
These calls were identified with `arc lint --everything` after enabling the lint rule in D21763.
Test Plan: Read strings, ran "arc".
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21764
Summary:
Ref T13658. One challenge in forking Phabricator is product name references in user-visible strings, particularly in "pht()".
Add a linter to identify the use of product name literals in "pht()" text.
The linter could fix these automatically, but it looks like there are fewer than 200 across both Arcanist and Phabricator and some sampling suggests that many are probably clearer when rewritten into generic language anyway.
Test Plan: Ran linter, saw it pop out reasonable warnings.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21763
Summary: Ref T13588. Adds "phutil_nonempty_string()" and similar methods to support PHP8.1 compatibility.
Test Plan: Added unit tests, ran unit tests.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21762
Summary:
Updated the mercurial script to pull the `parseurl` function from the new module if pulling from the old module fails.
Also updated pulling of `remoteopts` to follow the same pattern of fallback.
Fixes T13672
Test Plan:
Using mercurial 6.1 I ran `arc patch Dnnnnn --trace` and verified that it succeeded and in the trace output it used the `arc-ls-markers` extension.
Using mercurial 4.7 I ran `arc patch Dnnnnn --trace` and verified that it succeeded and in the trace output it used the `arc-ls-markers` extension.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13672
Differential Revision: https://secure.phabricator.com/D21747
Summary: Ref T13588. I pointed my local `php` at PHP 8.1 and this is what I've hit so far; all these cases seem very unlikely to have any subtle behavior.
Test Plan: Ran various `arc` workflows under PHP 8.1.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21742
Summary:
Ref T13588. "each()" has been a bad idea for a long time, and was formally deprecated in PHP 7.2 and removed in PHP 8.0.
Catch use of "each()" in lint and reject it.
Test Plan:
Added and ran unit tests.
{F10021268}
Subscribers: cspeckmim
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21741
Summary:
Ref T13588. See that task for discussion.
Improve behavior under PHP8.1, particularly the deprecation warning raised by calling `strlen(null)`.
Test Plan:
- Ran `arc help`, `arc branches`, `arc diff`, etc., under PHP 8.1 and PHP 7.4.
- Created this change with PHP8.1.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21740
Summary:
This default CA bundle file hasn't been updated since 2016. Update it to the current cURL extraction.
I believe this is notably impactful because of a new "Let's Encrypt" certificate, but didn't hunt down the particulars.
Test Plan:
Confirmed the hash matches the published hash:
```
$ openssl dgst -sha256 resources/ssl/default.pem
SHA256(resources/ssl/default.pem)= ae31ecb3c6e9ff3154cb7a55f017090448f88482f0e94ac927c0c67a1f33b9cf
```
This assurance is fairly meaningless since both the hash and file are published on `curl.se`. It didn't get corrupted by stellar radiation before it made it into Git, at least?
Differential Revision: https://secure.phabricator.com/D21739
