Page MenuHomePhorge

Fix early exit from getMatchSeverity on null severity
ClosedPublic

Authored by goddenrich on Apr 18 2023, 17:56.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 18:04
Unknown Object (File)
Sun, Mar 24, 06:18
Unknown Object (File)
Thu, Mar 21, 14:01
Unknown Object (File)
Thu, Mar 21, 07:28
Unknown Object (File)
Wed, Mar 20, 15:51
Unknown Object (File)
Wed, Mar 20, 10:24
Unknown Object (File)
Feb 17 2024, 20:11
Unknown Object (File)
Feb 17 2024, 20:11

Details

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

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/lint/linter/ArcanistScriptAndRegexLinter.php
379

Thanks for this patch.

Probably we should use !isset() instead of empty() since:

Valueif()empty()isset()
nullfalsetruefalse
0falsetruetrue
0.0falsetruetrue
"0"falsetruetrue
""falsetruetrue
falsefalsetruetrue
array()falsetruetrue
Everything elsetruefalsetrue

In short, empty() also discards the string "0" and maybe is important here.

https://we.phorge.it/book/flavor/article/php_pitfalls/

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

goddenrich added inline comments.
src/lint/linter/ArcanistScriptAndRegexLinter.php
379

Sure thing that makes a lot of sense.

goddenrich marked an inline comment as done.

addressing other possible nulls in this linter

This revision is now accepted and ready to land.Apr 19 2023, 15:08
valerio.bozzolan retitled this revision from early exit from getMatchSeverity on null severity to Fix early exit from getMatchSeverity on null severity.Apr 19 2023, 15:10
valerio.bozzolan edited the summary of this revision. (Show Details)

(Any commit is welcome to fix that damn linter, too)

This revision was landed with ongoing or failed builds.Apr 19 2023, 16:21
This revision was automatically updated to reflect the committed changes.