Summary: I found a few strings that I had missed, using a mostly-broken-but-somewhat-okay custom linter ruler (https://secure.phabricator.com/differential/diff/30988/).
Test Plan: Intense eyeballing.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aurelijus, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12888
Summary:
Currently, a lot of `ArcanistExternalLinter` subclasses have something like this:
```lang=php
if ($err && !$messages) {
return false;
}
```
We can avoid this code duplication by moving this check to the `ArcanistExternalLinter` class.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11321
Summary: This allows `ArcanistBaseXHPASTLinter::getXHPASTTreeForPath` to return the parse tree for a file that wasn't explicitly linted. In my particular use case, I am writing a linter rule to determine if it is necessary to import a PHP file with `require_once` (i.e. the file consists only of class/interface definitions which are autoloadable).
Test Plan: Tested externally using a custom linter.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12271
Summary: Ref T7674. `ArcanistXHPASTLinter` has some workarounds for running in "commit hook" mode (which has since been removed). This linter should always have a working copy.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7674
Differential Revision: https://secure.phabricator.com/D12956
Summary:
Removes the deprecated method of configuring linters (via the `.arcconfig` file). There is one main caveat here:
- There is currently no convenient method by which to change the path for an external linter (T5057). This means that there is no direct replacement for the deprecated `lint.ruby.prefix` configuration. The workaround is to symlink these binaries into `arcanist/externals/bin`.
Test Plan: Wait a sufficient amount of time before landing this.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aik099, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10005
Summary: Add some linter rule to detect invalid modifiers for class methods/properties.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12922
Summary: Ref T7977. The `ArcanistTestCase` class is pointless and can be replaced by `ArcanistPhutilTestCase`. Furthermore, it sorta makes sense to just rename `ArcanistPhutilTestCase` to `PhutilTestCase`. Depends on D12664 and D12666.
Test Plan: `arc unit`
Reviewers: avivey, #blessed_reviewers, epriestley
Reviewed By: avivey, #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12665
Summary:
Ref T7977. The `PhutilTestCase::getLink` method currently relies on arcanist projects instead of repositories. Instead, make this logic a bit smarter by looking up the base URI from `phabricator.uri` (currently it is hardcoded to `https://secure.phabricator.com`).
Ideally, we would pass `?repositories=$REPOSITORY_PHID` to `DiffusionSymbolController` as well, but I don't know if this is worth pursuing.
Test Plan: This diff.
Reviewers: avivey, #blessed_reviewers, epriestley
Reviewed By: avivey, #blessed_reviewers, epriestley
Subscribers: aurelijus, Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12664
Summary: Improve the `ArcanistXHPASTLinter::LINT_ALIAS_FUNCTION` linter rule. Currently this rule does not correctly handle alias functions which are not strictly lowercase.
Test Plan: Added a test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12921
Summary: Fixes T7417. Adds `ArcanistXHPASTLinter::LINT_MODIFIER_ORDERING` for ensuring that method/property modifiers (`public`, `protected`, `private`, `static`, `abstract` and `final`) are consistently ordered. The modifier ordering that is enforced is consistent with [[http://www.php-fig.org/psr/psr-2 | PSR-2]].
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7417
Differential Revision: https://secure.phabricator.com/D12421
Summary:
Improve the `ArcanistXHPASTLinter::LINT_ARRAY_SEPARATOR` rule to be able to autofix the following code:
```lang=php
array(
1,
2,
3, /* comment */ );
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12308
Summary: Depends on D9430. If we are using the system installation of `pep8`, then it probably unnecessary (and possibly wrong) to specify `python2.6` as the default interpreter.
Test Plan: Ran `arc unit`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: vrusinov, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9474
Summary: Currently, we bundle a specific version of `pep8` with Arcanist. Whilst maybe this made sense historically, as pointed out by @epriestley in D9412#6, there is no longer much point in doing so.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: talshiri, vrusinov, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D9430
Summary: This linter is a relic from Facebook days. As Harbormaster becomes more useful (T1049) this linter should become obsolete. Instead of modernising this linter to be compatible with modern infrastructure, it is easier to just let it die.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10533
Summary: Move the `getFunctionCalls` method to the `ArcanistBaseXHPASTLinter` so that it can be used by subclasses.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12472
Summary: Currently all CPPLint issues are hard-coded to warning level, which prevents customising the severity in .arclint. Change to pick up the configured severity. Note that getLintMessageSeverity will call getDefaultMessageSeverity if nothing is configured for that error category.
Test Plan: Tested manually to confirm configured categories display with the correct severity and that non-configured ones return with the default severity (ERROR).
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9682
Summary: The `instanceof` operator expects the first argument to be an object instance. See http://www.phpwtf.org/instanceof-smart.
Test Plan: Added test case
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12856
Summary:
The `ArcanistXHPASTLinter::LINT_TOSTRING_EXCEPTION` linter rule was introduced in D12854, but fails for the following:
- Interfaces which declare the `__toString()` method.
- Classes which mark the `__toString()` method as `abstract`.
Test Plan: Added test cases.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12892
Summary:
See http://phpsadness.com/sad/39. Declaring a function named `__lambda_func` prevents the `create_function` function from working. This is because `create_function` eval-declares the function `__lambda_func`, then modifies the symbol table so that the function is instead named `"\0lambda_".(++$i)`, and returns that name.
NOTE: Personally, I don't think that anyone should use `create_function`. However, despite this, I think it is reasonable that no function is named `__lambda_func` in case some external library relies on `create_function`.
Test Plan: Added test case.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12870
Summary: Fixes T8207. It is not possible to throw an exception from within the `__toString()` method. Add a linter rule to prevent this from happening.
Test Plan: Added test cases.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8207
Differential Revision: https://secure.phabricator.com/D12854
Summary: Currently the error message from `ArcanistXHPASTLinter::LINT_FORMATTED_STRING` is slightly wrong, mentioning `xsprintf` instead of the actual `sprintf`-style function which was used. This diff changes the error message to be more correct.
Test Plan: Linted a file containing `sprintf('%s')` and saw a reasonable lint message.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12871
Summary: This linter rule was introduced in D12420, but fails to handle a specific edge case.
Test Plan: Added test case.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12853
Summary: Ref T5655. After D9983, `ArcanistBaseWorkflow` and `ArcanistBaseUnitTestEngine` are deprecated.
Test Plan: Wait a sufficient amount of time before landing this.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D10004
Summary: Ref T7409. This linter rule was added in D12419, but fails in some specific cases. Improve the handling of `n_DECLARATION_PARAMETER_LIST`.
Test Plan: Added unit tests.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7409
Differential Revision: https://secure.phabricator.com/D12828
Summary: This lost formatting in a pht() conversion; give the bold back and make it properly translatable.
Test Plan: Will land.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12843
Summary: Refs D12607. Fixes T8195. Replace period in the sprintf() arguments with a comma.
Test Plan: Ran `arc diff` in the patch applied, did not get the sprintf() error
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T8195
Differential Revision: https://secure.phabricator.com/D12837
Summary:
The following code is invalid:
```
final class MyClass {
public function __construct() {
parent::__construct(null);
}
}
$x = new MyClass();
```
Running the above code will produce a fatal error:
```
PHP Fatal error: Cannot access parent:: when current class scope has no parent
```
Test Plan: Added unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12420
Summary:
Ref T2039. Convert the `ArcanistPylintLinter` to an `ArcanistExternalLinter` and make it compatible with `.arclint`. In doing so, it was necessary to drop support of older versions of `pylint` by setting the minimum version to be 1.0.0. I think that dropping support for older versions is reasonable because version 1.0.0 was released ~18 months ago. In the case than an incompatible version is detected, an `ArcanistMissingLinterException` is thrown.
One caveat here is that support for `lint.pylint.codes.(error|warning|advice)` is dropped. Any installs that are relying on this configuration will need to migrate to using the `.arclint` file for specifying linter severity. We could potentially continue to support this deprecated configuration, but I'm not sure if this is worthwhile.
Test Plan: Wrote and executed unit tests with `arc unit`. I ran the unit tests on all `pylint` releases after (and including) 1.0.0. Specifically, the tests were run on v1.0.0, v1.1.0, v1.2.0, v1.3.0 and v1.4.0.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2039
Differential Revision: https://secure.phabricator.com/D9109
Summary: Use `__CLASS__` instead of hardcoding class names. Depends on D12804.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12805
Summary: Add a linter rule to advise against the use of hardcoded class names. Hardcoded class names make the code harder to refactor and it is generally preferable to use the `__CLASS__` magic constant instead.
Test Plan: This works, but needs some polish.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12605
Summary: Ref T8087. Prepares for eventually making these optional after T6030. See also T7443.
Test Plan:
- See the next change for the server-side part of this.
- With both patches applied, rigged the server to return `D123`.
- Created a revision, saw bare `D123`.
- Updated it with bare `D123`, things worked properly.
- Created this revision with full URIs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: cburroughs, epriestley
Maniphest Tasks: T8087
Differential Revision: https://secure.phabricator.com/D12748
Summary:
Ref T5955. This logic is a little cleaner than the previous version.
Don't require `~/.arcrc` to exist if the caller provides `--conduit-token`.
Test Plan:
- Made calls with `--conduit-token` and no `~/.arcrc`.
- Made calls with `--conduit-token` and a normal `~/.arcrc`.
- Made calls with normal `arc`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mbishopim3, epriestley
Maniphest Tasks: T5955
Differential Revision: https://secure.phabricator.com/D12750
Summary:
The ArcanistPhpunitTestResultParser has been changed
to use phutil_json_decode instead of json_decode in
rARCa4d33ef117aa8702181154b60ce1ce52bcfc119b
That has broken the functionality as json_decode has returned
an object before but phutil_json_decode does return an array.
The code has now been adopted to work with the array result
instead of an object.
Test Plan: Run a phpunit test case
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aurelijus, epriestley
Differential Revision: https://secure.phabricator.com/D12751
Summary: Fixes T6311 and T5124 by returning all configured linters from `buildLinter()`, and making `ArcanistExternalLinter::checkBinaryConfiguration()` not crash if there's no executable to run.
Test Plan: `arc linters` in rP shows "Configured" and "ERROR" as appropriate; Adding a broken linter to `.arclint` in rARC doesn't invoke it's not actually needed, and prints error if it is.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: joshuaspence, epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6311
Differential Revision: https://secure.phabricator.com/D10773
Summary: Ref T5955. This makes it easier to write scripts which call Conduit via `arc call-conduit`.
Test Plan: Used `arc --conduit-token ... call-conduit user.whoami` to make calls as various users.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mbishopim3, epriestley
Maniphest Tasks: T5955
Differential Revision: https://secure.phabricator.com/D12717
Summary: Support and prefer configuration from .arclint over Configuration Manager
Test Plan: arc lint with several combinations of values in .arcconfig and .arclint.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: joshuaspence, epriestley, #blessed_reviewers
Subscribers: vrusinov, jpoehls, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10704
Summary: Ref T7674. Remove support for passing data to linters via `STDIN`. This functionality exists primarily for pre-receive workflows (which don't have a working copy to act on), but these workflows are going away soon.
Test Plan: `arc unit`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7674
Differential Revision: https://secure.phabricator.com/D12696
