Details
- Reviewers
valerio.bozzolan - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15190: PHP 8.1: strlen() and other scalar-only functions do not accept NULL anymore - understand fix strategies
T15257: arc lint broken on php 8.2 - Commits
- rARC75af13abe917: Fix early exit from getMatchSeverity on null severity
I ran this with an arc linter on a private repository which doesn't have a regex to match severity
Diff Detail
- Repository
- rARC Arcanist
- Branch
- master
- Lint
Lint Passed - Unit
Test Failures - Build Status
Buildable 255 Build 255: arc lint + arc unit
| Time | Test | |
|---|---|---|
| 194 ms | ArcanistPuppetLintLinterTestCase::testLinter Assertion failed, expected values to be equal (at ArcanistLinterTestCase.php:180): Some linters failed:
- CommandException: Command failed with error #1!
COMMAND
| |
| 9 ms | ArcanistXMLLinterTestCase::testLinter Lint emitted an unexpected set of messages for file "languages-6.lint-test".
MISSING MESSAGES
Message with severity "error" at "3:16" (XML76)
| |
| 6 ms | ArcanistChmodLinterTestCase::testLinter 5 assertion(s) passed. | |
| 0 ms | ArcanistClosureLinterTestCase::testVersion 1 assertion(s) passed. | |
| 0 ms | ArcanistCpplintLinterTestCase::testVersion 1 assertion(s) passed. | |
| View Full Test Results (2 Failed · 25 Passed · 26 Skipped) |
Event Timeline
| src/lint/linter/ArcanistScriptAndRegexLinter.php | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 379 | Thanks for this patch. Probably we should use !isset() instead of empty() since:
In short, empty() also discards the string "0" and maybe is important here. | |||||||||||||||||||||||||||||||||||||
| src/lint/linter/ArcanistScriptAndRegexLinter.php | ||
|---|---|---|
| 379 | thanks yep that makes sense. Im still pretty unaware of the quirks of php | |
looking at those failing test cases puppet-lint for my version puppet-lint 2.4.2 uses
diff --git a/src/lint/linter/ArcanistPuppetLintLinter.php b/src/lint/linter/ArcanistPuppetLintLinter.php index 8336cd42..5b78ffd0 100644 --- a/src/lint/linter/ArcanistPuppetLintLinter.php +++ b/src/lint/linter/ArcanistPuppetLintLinter.php @@ -56,7 +56,7 @@ final class ArcanistPuppetLintLinter extends ArcanistExternalLinter { return array( '--error-level=all', sprintf('--log-format=%s', implode('|', array( - '%{linenumber}', + '%{line}', '%{column}', '%{kind}', '%{check}',
and also doesn't seem to correctly pick up quote boolean warnings.
xml linter seems to now specify a different column for one test
rgodden@rgodden-tm05522:~/repos/arcanist$ git diff src/lint/linter/__tests__/ diff --git a/src/lint/linter/__tests__/xml/languages-6.lint-test b/src/lint/linter/__tests__/xml/languages-6.lint-test index f652fbb8..7f14c97f 100644 --- a/src/lint/linter/__tests__/xml/languages-6.lint-test +++ b/src/lint/linter/__tests__/xml/languages-6.lint-test @@ -3,5 +3,5 @@ </lang> </languages> ~~~~~~~~~~ -error:3:16:XML76:LibXML Error +error:3:12:XML76:LibXML Error error:4:1:XML5:LibXML Error
| src/lint/linter/ArcanistScriptAndRegexLinter.php | ||
|---|---|---|
| 379 | At the moment the above line ↑ means this ↓ $severity_name = strtolower($match['severity'] ?? null); So, if somebody used idx() it means that this value can be undefined/NULL. So, before PHP 8.1 NULL was not a problem, since $severity_name was just casted to an empty string by strtolower(). In short this function probably must work even if $severity_name is an empty string "". | |
| 379 | Sorry if I propose another approach 🙈 Can we rollback violently and try the above one-line change instead? Does this make your day too? I propose this, since I'm not a Phorge maintainer and I don't 100% understand all the facets in this method. But, I can reasonably assume that the author used idx() for a reason: the author assumed that $match['severity'] can be undefined. So, that value can become NULL. So, NULL was passed to strtolower() and - before PHP 8.1 - that was good, and the problem is: now it's not. So we can just explicit a default valid value (an empty string, instead of a deprecated NULL), without changing any other logic and without implementing any new short-circuit, and just relying on the previous don't care strtolower('') logic. (Ideally, we would not call strtolower() with an empty string, but, again, it's probably a reasonable don't care there). I'm not bold enough to approve the previous change, but I will surely approve this patch using the idx($match, 'severity', '') since to me that is the exact minimal change to make PHP 8.1 happy, and gives to me a 100.0000% certainty that we will not introduce any regression (instead of 90%) - assuming that the code that was already there, already made sense. Thank you for your thoughts in this and for your precious help | |
| src/lint/linter/ArcanistScriptAndRegexLinter.php | ||
|---|---|---|
| 379 | Sure thing that makes a lot of sense. | |
