Page MenuHomePhabricator

Fix a PHP 8.1 issue in lint rendering
Needs RevisionPublic

Authored by epriestley on Dec 11 2021, 7:17 PM.

Details

Reviewers
0
Maniphest Tasks
T13588: PHP 8 Compatibility
Summary

Ref T13588. The lint renderer has some "strlen(null)" issues; correct them.

Test Plan

Hit lint messages under PHP 8.1.

Diff Detail

Repository
rARC Arcanist
Branch
lint1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 25614
Build 35431: Run Core Tests
Build 35430: arc lint + arc unit

Event Timeline

0 requested changes to this revision.Dec 19 2021, 5:56 AM
0 added a subscriber: 0.
0 added inline comments.
src/lint/renderer/ArcanistConsoleLintRenderer.php
112

In PHP 8.1, I think this call will fail whenever the original text is null and the replacement text isn't. Though that's probably not a legitimate arrangement.

115

This shouldn't change the function's behaviour in PHP 7.4 and 8.0, as long as the original text is either null or a string. However, a short comment might be useful here, since this line does make me wonder what the original text gets up to when it's not busy being a string.

This revision now requires changes to proceed.Dec 19 2021, 5:56 AM

There's a similar PHP 8.1 related deprecation notice in the Unit renderer on this line:

https://secure.phabricator.com/diffusion/ARC/browse/master/src/unit/renderer/ArcanistUnitConsoleRenderer.php$15

$test_name = $result->getName();
$test_namespace = $result->getNamespace();
if (strlen($test_namespace)) {
  $test_name = $test_namespace.'::'.$test_name;
}

If the test result's namespace isn't set, getNamespace will return null, which will cause a deprecation notice in the strlen call:

[2021-12-29 12:50:43] EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]

#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:189]
#2 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:419]

Casting to a string should resolve:

$test_name = $result->getName();
$test_namespace = phutil_string_cast($result->getNamespace());
if (strlen($test_namespace)) {
  $test_name = $test_namespace.'::'.$test_name;
}

It's been a while since I've created a diff against secure.phabricator.com and I am not setup currently to do it, let me know if that would be preferred (or at the very least a new ticket).

Thanks!

There's also an error in Phabricator in src/infrastructure/env/PhabricatorEnv.php that can be fixed likewise with a similar diff:

--- a/src/infrastructure/env/PhabricatorEnv.php
+++ b/src/infrastructure/env/PhabricatorEnv.php
@@ -124,7 +124,7 @@ final class PhabricatorEnv extends Phobject {
 
     // If an instance identifier is defined, write it into the environment so
     // it's available to subprocesses.
-    $instance = self::getEnvConfig('cluster.instance');
+    $instance = phutil_string_cast(self::getEnvConfig('cluster.instance'));
     if (strlen($instance)) {
       putenv('PHABRICATOR_INSTANCE='.$instance);
       $_ENV['PHABRICATOR_INSTANCE'] = $instance;

Alternatively using $instance !== null one can achieve what seems to be a similar test for null, but there's probably some reason that wasn't used in the first place.