Changeset View
Standalone View
src/lint/linter/ArcanistScriptAndRegexLinter.php
| Show First 20 Lines • Show All 197 Lines • ▼ Show 20 Lines | /* -( Linting )------------------------------------------------------------ */ | ||||||||||||||||||||||||||||||||||||||||||||||
| } | } | ||||||||||||||||||||||||||||||||||||||||||||||
| /** | /** | ||||||||||||||||||||||||||||||||||||||||||||||
| * Run the regex on the output of the script. | * Run the regex on the output of the script. | ||||||||||||||||||||||||||||||||||||||||||||||
| * | * | ||||||||||||||||||||||||||||||||||||||||||||||
| * @task lint | * @task lint | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | */ | ||||||||||||||||||||||||||||||||||||||||||||||
| public function lintPath($path) { | public function lintPath($path) { | ||||||||||||||||||||||||||||||||||||||||||||||
| $output = idx($this->output, $path); | $output = idx($this->output, $path, ''); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!strlen($output)) { | if (!strlen($output)) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // No output, but it exited 0, so just move on. | // No output, but it exited 0, so just move on. | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | } | ||||||||||||||||||||||||||||||||||||||||||||||
| $matches = null; | $matches = null; | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!preg_match_all($this->regex, $output, $matches, PREG_SET_ORDER)) { | if (!preg_match_all($this->regex, $output, $matches, PREG_SET_ORDER)) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Output with no matches. This might be a configuration error, but more | // Output with no matches. This might be a configuration error, but more | ||||||||||||||||||||||||||||||||||||||||||||||
| ▲ Show 20 Lines • Show All 117 Lines • ▼ Show 20 Lines | /* -( Parsing Output )----------------------------------------------------- */ | ||||||||||||||||||||||||||||||||||||||||||||||
| private function getMatchLineAndChar(array $match, $path) { | private function getMatchLineAndChar(array $match, $path) { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!empty($match['offset'])) { | if (!empty($match['offset'])) { | ||||||||||||||||||||||||||||||||||||||||||||||
| list($line, $char) = $this->getEngine()->getLineAndCharFromOffset( | list($line, $char) = $this->getEngine()->getLineAndCharFromOffset( | ||||||||||||||||||||||||||||||||||||||||||||||
| idx($match, 'file', $path), | idx($match, 'file', $path), | ||||||||||||||||||||||||||||||||||||||||||||||
| $match['offset']); | $match['offset']); | ||||||||||||||||||||||||||||||||||||||||||||||
| return array($line + 1, $char + 1); | return array($line + 1, $char + 1); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | } | ||||||||||||||||||||||||||||||||||||||||||||||
| $line = idx($match, 'line'); | $line = idx($match, 'line', ''); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (strlen($line)) { | if (strlen($line)) { | ||||||||||||||||||||||||||||||||||||||||||||||
| $line = (int)$line; | $line = (int)$line; | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!$line) { | if (!$line) { | ||||||||||||||||||||||||||||||||||||||||||||||
| $line = 1; | $line = 1; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | } | ||||||||||||||||||||||||||||||||||||||||||||||
| } else { | } else { | ||||||||||||||||||||||||||||||||||||||||||||||
| $line = null; | $line = null; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | } | ||||||||||||||||||||||||||||||||||||||||||||||
| Show All 22 Lines | private function getMatchSeverity(array $match) { | ||||||||||||||||||||||||||||||||||||||||||||||
| $map = array( | $map = array( | ||||||||||||||||||||||||||||||||||||||||||||||
| 'error' => ArcanistLintSeverity::SEVERITY_ERROR, | 'error' => ArcanistLintSeverity::SEVERITY_ERROR, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'warning' => ArcanistLintSeverity::SEVERITY_WARNING, | 'warning' => ArcanistLintSeverity::SEVERITY_WARNING, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'autofix' => ArcanistLintSeverity::SEVERITY_AUTOFIX, | 'autofix' => ArcanistLintSeverity::SEVERITY_AUTOFIX, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'advice' => ArcanistLintSeverity::SEVERITY_ADVICE, | 'advice' => ArcanistLintSeverity::SEVERITY_ADVICE, | ||||||||||||||||||||||||||||||||||||||||||||||
| 'disabled' => ArcanistLintSeverity::SEVERITY_DISABLED, | 'disabled' => ArcanistLintSeverity::SEVERITY_DISABLED, | ||||||||||||||||||||||||||||||||||||||||||||||
| ); | ); | ||||||||||||||||||||||||||||||||||||||||||||||
| $severity_name = strtolower(idx($match, 'severity')); | $severity_name = strtolower(idx($match, 'severity', '')); | ||||||||||||||||||||||||||||||||||||||||||||||
valerio.bozzolan: At the moment the above line ↑ means this ↓
```lang=php
$severity_name = strtolower($match… | |||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
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. valerio.bozzolan: Thanks for this patch.
Probably we should use `!isset()` instead of `empty()` since:
| Value… | |||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actionsthanks yep that makes sense. Im still pretty unaware of the quirks of php goddenrich: thanks yep that makes sense. Im still pretty unaware of the quirks of php | |||||||||||||||||||||||||||||||||||||||||||||||
Done Inline Actions
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 valerio.bozzolan: Sorry if I propose another approach 🙈
Can we rollback violently and try the above one-line… | |||||||||||||||||||||||||||||||||||||||||||||||
Done Inline ActionsSure thing that makes a lot of sense. goddenrich: Sure thing that makes a lot of sense. | |||||||||||||||||||||||||||||||||||||||||||||||
| foreach ($map as $name => $severity) { | foreach ($map as $name => $severity) { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!empty($match[$name])) { | if (!empty($match[$name])) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return $severity; | return $severity; | ||||||||||||||||||||||||||||||||||||||||||||||
| } else if ($severity_name == $name) { | } else if ($severity_name == $name) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return $severity; | return $severity; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | } | ||||||||||||||||||||||||||||||||||||||||||||||
| return ArcanistLintSeverity::SEVERITY_ERROR; | return ArcanistLintSeverity::SEVERITY_ERROR; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | } | ||||||||||||||||||||||||||||||||||||||||||||||
At the moment the above line ↑ means this ↓
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 "".