Fixes to make arcanist compatible with php8.1 on Linux

[hiraditya]

Summary

php8.1

• introduced strict type checking and error reporting
• removed deprecated APIs

Also has changes suggested in:
https://discourse.llvm.org/t/arcanist-deprecation-errors-on-php-8/63231

PR: https://github.com/llvm/llvm-project/issues/57271

Testing:

sh
cd /usr/share/arcanist
patch -p1 < /path/to/llvm-project/llvm/utils/phabricator/0001-Fix-PHP-8.1-incompatibility-with-arc-patch-D-12345.patch
patch -p1 < https://reviews.llvm.org/file/data/ck5dddpgtnurs5jjivr2/PHID-FILE-krs7drhq43t4p7jciltf/D131699.diff
patch -p1 < https://reviews.llvm.org/file/data/2g2fs6frlcha5cglbdns/PHID-FILE-laajtlfzcpnharrrp7un/D132523.diff

arc diff

[hiraditya]

Adding more fixes.

[hiraditya]

Added all the changes

[hiraditya]

Make it git compatible.

[nickdesaulniers]

Does the description in phab need updating? It suggests multiple invocations of patch?

[hiraditya]

In D132523#3749655, @nickdesaulniers wrote:

Does the description in phab need updating? It suggests multiple invocations of patch?

Yes. The instructions were for anyone to try this patch while in review.

I guess once https://reviews.llvm.org/D131699 is merged, i'll update the description.

[pmatos]

Thanks for this. I needed this patch for arc to work again.

[nickdesaulniers]

The resulting patch fails to apply to arcanist for me. STR:

$ cd llvm-project
$ wget https://reviews.llvm.org/file/data/nrt6tovpa573cdwctpyc/PHID-FILE-kgufiuwoyczdbfqdezq5/D132523.diff
$ git apply D132523.diff
$ cd arcanist
$ git am /android0/llvm-project/llvm/utils/phabricator/*.patch
Applying: Fix PHP 8.1 incompatibility with `arc patch D<12345>`
Applying: Fixes to make arcanist compatible with php8.1
error: patch failed: src/error/PhutilErrorHandler.php:180
error: src/error/PhutilErrorHandler.php: patch does not apply
error: patch failed: src/lint/engine/ArcanistLintEngine.php:274
error: src/lint/engine/ArcanistLintEngine.php: patch does not apply
error: patch failed: src/lint/linter/ArcanistLinter.php:291
error: src/lint/linter/ArcanistLinter.php: patch does not apply
error: patch failed: src/moduleutils/PhutilLibraryMapBuilder.php:122
error: src/moduleutils/PhutilLibraryMapBuilder.php: patch does not apply
error: patch failed: src/object/Phobject.php:33
error: src/object/Phobject.php: patch does not apply
error: patch failed: src/parser/PhutilTypeSpec.php:76
error: src/parser/PhutilTypeSpec.php: patch does not apply
error: patch failed: src/unit/engine/phutil/PhutilTestCase.php:408
error: src/unit/engine/phutil/PhutilTestCase.php: patch does not apply
error: patch failed: src/utils/PhutilArray.php:29
error: src/utils/PhutilArray.php: patch does not apply
error: patch failed: src/utils/utils.php:2003
error: src/utils/utils.php: patch does not apply
error: patch failed: src/workflow/ArcanistWorkflow.php:728
error: src/workflow/ArcanistWorkflow.php: patch does not apply
Patch failed at 0002 Fixes to make arcanist compatible with php8.1
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

This patch needs to be able to be applied on top of llvm/utils/phabricator/0001-Fix-PHP-8.1-incompatibility-with-arc-patch-D-12345.patch.

[hiraditya]

not needed anymore.

llvm/utils/phabricator/0002-Fixes-to-make-arcanist-compatible-with-php8.1.patch

+From 5c8a81b692b62ce552f7331b5009f95c2c36070c Mon Sep 17 00:00:00 2001
+From: Aditya Kumar <[e-mail]>
+Date: Tue, 23 Aug 2022 20:16:39 -0700
+Subject: [PATCH] Fixes to make arcanist compatible with php8.1
+
+---
+ src/error/PhutilErrorHandler.php            |   2 +-
+ src/lint/engine/ArcanistLintEngine.php      |   4 +-
+ src/lint/linter/ArcanistLinter.php          |   2 +-
+ src/moduleutils/PhutilLibraryMapBuilder.php |   2 +-
+ src/object/Phobject.php                     |   5 +
+ src/parser/PhutilTypeSpec.php               |   7 +-
+ src/repository/api/ArcanistGitAPI.php       |   3 +
+ src/unit/engine/phutil/PhutilTestCase.php   |  16 +--
+ src/utils/PhutilArray.php                   |   5 +
+ src/utils/utils.php                         | 125 ++++++++++++++++++++
+ src/workflow/ArcanistWorkflow.php           |   4 +-
+ support/init/init-script.php                |   2 +-
+ 12 files changed, 159 insertions(+), 18 deletions(-)
+
+diff --git a/src/error/PhutilErrorHandler.php b/src/error/PhutilErrorHandler.php
+index deb6ee1..bdae1f7 100644
+--- a/src/error/PhutilErrorHandler.php
++++ b/src/error/PhutilErrorHandler.php
+@@ -180,7 +180,7 @@ final class PhutilErrorHandler extends Phobject {
+    * @return void
+    * @task internal
+    */
+-  public static function handleError($num, $str, $file, $line, $ctx) {
++  public static function handleError($num, $str, $file, $line, $ctx = 'E_NOCONTEXT') {
+ 
+     foreach (self::$traps as $trap) {
+       $trap->addError($num, $str, $file, $line, $ctx);
+diff --git a/src/lint/engine/ArcanistLintEngine.php b/src/lint/engine/ArcanistLintEngine.php
+index f737fcd..36f686f 100644
+--- a/src/lint/engine/ArcanistLintEngine.php
++++ b/src/lint/engine/ArcanistLintEngine.php
+@@ -274,7 +274,7 @@ abstract class ArcanistLintEngine extends Phobject {
+     return ArcanistLintSeverity::isAtLeastAsSevere($severity, $minimum);
+   }
+ 
+-  final private function shouldUseCache(
++  /*final*/ private function shouldUseCache(
+     $cache_granularity,
+     $repository_version) {
+ 
+@@ -313,7 +313,7 @@ abstract class ArcanistLintEngine extends Phobject {
+     return $this;
+   }
+ 
+-  final private function isRelevantMessage(ArcanistLintMessage $message) {
++  /*final*/ private function isRelevantMessage(ArcanistLintMessage $message) {
+     // When a user runs "arc lint", we default to raising only warnings on
+     // lines they have changed (errors are still raised anywhere in the
+     // file). The list of $changed lines may be null, to indicate that the
+diff --git a/src/lint/linter/ArcanistLinter.php b/src/lint/linter/ArcanistLinter.php
+index afb8cbe..d4aec13 100644
+--- a/src/lint/linter/ArcanistLinter.php
++++ b/src/lint/linter/ArcanistLinter.php
+@@ -291,7 +291,7 @@ abstract class ArcanistLinter extends Phobject {
+    * @param  list<string>
+    * @return list<string>
+    */
+-  final private function filterPaths(array $paths) {
++  /*final*/ private function filterPaths(array $paths) {
+     $engine = $this->getEngine();
+ 
+     $keep = array();
+diff --git a/src/moduleutils/PhutilLibraryMapBuilder.php b/src/moduleutils/PhutilLibraryMapBuilder.php
+index c036ed4..23a496f 100644
+--- a/src/moduleutils/PhutilLibraryMapBuilder.php
++++ b/src/moduleutils/PhutilLibraryMapBuilder.php
+@@ -122,7 +122,7 @@ final class PhutilLibraryMapBuilder extends Phobject {
+    */
+   private function log($message) {
+     if (!$this->quiet) {
+-      @fwrite(STDERR, "%s\n", $message);
++      @fwrite(STDERR, sprintf("%s\n", $message));
+     }
+     return $this;
+   }
+diff --git a/src/object/Phobject.php b/src/object/Phobject.php
+index 0a7a59e..4d79930 100644
+--- a/src/object/Phobject.php
++++ b/src/object/Phobject.php
+@@ -33,22 +33,27 @@ abstract class Phobject implements Iterator {
+         get_class($this).'::'.$name));
+   }
+ 
++  #[\ReturnTypeWillChange]
+   public function current() {
+     $this->throwOnAttemptedIteration();
+   }
+ 
++  #[\ReturnTypeWillChange]
+   public function key() {
+     $this->throwOnAttemptedIteration();
+   }
+ 
++  #[\ReturnTypeWillChange]
+   public function next() {
+     $this->throwOnAttemptedIteration();
+   }
+ 
++  #[\ReturnTypeWillChange]
+   public function rewind() {
+     $this->throwOnAttemptedIteration();
+   }
+ 
++  #[\ReturnTypeWillChange]
+   public function valid() {
+     $this->throwOnAttemptedIteration();
+   }
+diff --git a/src/parser/PhutilTypeSpec.php b/src/parser/PhutilTypeSpec.php
+index c49fccf..466f874 100644
+--- a/src/parser/PhutilTypeSpec.php
++++ b/src/parser/PhutilTypeSpec.php
+@@ -76,8 +76,11 @@ final class PhutilTypeSpec extends Phobject {
+         break;
+       case 'regex':
+         $trap = new PhutilErrorTrap();
+-          $ok = @preg_match($value, '');
+-          $err = $trap->getErrorsAsString();
++        if (!is_string($value)) {
++          $value = implode(', ', $value);
++        }
++        $ok = @preg_match($value, '');
++        $err = $trap->getErrorsAsString();
+         $trap->destroy();
+ 
+         if ($ok === false) {
+diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php
+index dc5f116..66e8281 100644
+--- a/src/repository/api/ArcanistGitAPI.php
++++ b/src/repository/api/ArcanistGitAPI.php
+@@ -1140,6 +1140,9 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
+   }
+ 
+   public function hasLocalCommit($commit) {
++    if (!$commit) {
++      return false;
++    }
+     try {
+       if (!phutil_nonempty_string($commit) ||
+           !$this->getCanonicalRevisionName($commit)) {
+diff --git a/src/unit/engine/phutil/PhutilTestCase.php b/src/unit/engine/phutil/PhutilTestCase.php
+index fddb068..d296f79 100644
+--- a/src/unit/engine/phutil/PhutilTestCase.php
++++ b/src/unit/engine/phutil/PhutilTestCase.php
+@@ -408,7 +408,7 @@ abstract class PhutilTestCase extends Phobject {
+    *
+    * @task internal
+    */
+-  final private function failTest($reason) {
++  /*final*/ private function failTest($reason) {
+     $this->resultTest(ArcanistUnitTestResult::RESULT_FAIL, $reason);
+   }
+ 
+@@ -421,7 +421,7 @@ abstract class PhutilTestCase extends Phobject {
+    *
+    * @task internal
+    */
+-  final private function passTest($reason) {
++  /*final*/ private function passTest($reason) {
+     $this->resultTest(ArcanistUnitTestResult::RESULT_PASS, $reason);
+   }
+ 
+@@ -433,12 +433,12 @@ abstract class PhutilTestCase extends Phobject {
+    * @return void
+    * @task internal
+    */
+-  final private function skipTest($reason) {
++  /*final*/ private function skipTest($reason) {
+     $this->resultTest(ArcanistUnitTestResult::RESULT_SKIP, $reason);
+   }
+ 
+ 
+-  final private function resultTest($test_result, $reason) {
++  /*final*/ private function resultTest($test_result, $reason) {
+     $coverage = $this->endCoverage();
+ 
+     $result = new ArcanistUnitTestResult();
+@@ -553,7 +553,7 @@ abstract class PhutilTestCase extends Phobject {
+   /**
+    * @phutil-external-symbol function xdebug_start_code_coverage
+    */
+-  final private function beginCoverage() {
++  /*final*/ private function beginCoverage() {
+     if (!$this->enableCoverage) {
+       return;
+     }
+@@ -566,7 +566,7 @@ abstract class PhutilTestCase extends Phobject {
+    * @phutil-external-symbol function xdebug_get_code_coverage
+    * @phutil-external-symbol function xdebug_stop_code_coverage
+    */
+-  final private function endCoverage() {
++  /*final*/ private function endCoverage() {
+     if (!$this->enableCoverage) {
+       return;
+     }
+@@ -618,7 +618,7 @@ abstract class PhutilTestCase extends Phobject {
+     return $coverage;
+   }
+ 
+-  final private function assertCoverageAvailable() {
++  /*final*/ private function assertCoverageAvailable() {
+     if (!function_exists('xdebug_start_code_coverage')) {
+       throw new Exception(
+         pht("You've enabled code coverage but XDebug is not installed."));
+@@ -675,7 +675,7 @@ abstract class PhutilTestCase extends Phobject {
+    *
+    * @return map
+    */
+-  final private static function getCallerInfo() {
++  /*final*/ private static function getCallerInfo() {
+     $callee = array();
+     $caller = array();
+     $seen = false;
+diff --git a/src/utils/PhutilArray.php b/src/utils/PhutilArray.php
+index 8656bab..aedd65d 100644
+--- a/src/utils/PhutilArray.php
++++ b/src/utils/PhutilArray.php
+@@ -29,6 +29,7 @@ abstract class PhutilArray
+ /* -(  Countable Interface  )------------------------------------------------ */
+ 
+ 
++  #[\ReturnTypeWillChange]
+   public function count() {
+     return count($this->data);
+   }
+@@ -61,18 +62,22 @@ abstract class PhutilArray
+ /* -(  ArrayAccess Interface  )---------------------------------------------- */
+ 
+ 
++  #[\ReturnTypeWillChange]
+   public function offsetExists($key) {
+     return array_key_exists($key, $this->data);
+   }
+ 
++  #[\ReturnTypeWillChange]
+   public function offsetGet($key) {
+     return $this->data[$key];
+   }
+ 
++  #[\ReturnTypeWillChange]
+   public function offsetSet($key, $value) {
+     $this->data[$key] = $value;
+   }
+ 
++  #[\ReturnTypeWillChange]
+   public function offsetUnset($key) {
+     unset($this->data[$key]);
+   }
+diff --git a/src/utils/utils.php b/src/utils/utils.php
+index 4c44d7b..dc2099d 100644
+--- a/src/utils/utils.php
++++ b/src/utils/utils.php
+@@ -2003,3 +2003,128 @@ function phutil_partition(array $map) {
+ 
+   return $partitions;
+ }
++
++/**
++ * Test if a value is a nonempty string.
++ *
++ * The value "null" and the empty string are considered empty; all other
++ * strings are considered nonempty.
++ *
++ * This method raises an exception if passed a value which is neither null
++ * nor a string.
++ *
++ * @param Value to test.
++ * @return bool True if the parameter is a nonempty string.
++ */
++function phutil_nonempty_string($value) {
++  if ($value === null) {
++    return false;
++  }
++
++  if ($value === '') {
++    return false;
++  }
++
++  if (is_string($value)) {
++    return true;
++  }
++
++  throw new InvalidArgumentException(
++    pht(
++      'Call to phutil_nonempty_string() expected null or a string, got: %s.',
++      phutil_describe_type($value)));
++}
++
++
++/**
++ * Test if a value is a nonempty, stringlike value.
++ *
++ * The value "null", the empty string, and objects which have a "__toString()"
++ * method which returns the empty string are empty.
++ *
++ * Other strings, and objects with a "__toString()" method that returns a
++ * string other than the empty string are considered nonempty.
++ *
++ * This method raises an exception if passed any other value.
++ *
++ * @param Value to test.
++ * @return bool True if the parameter is a nonempty, stringlike value.
++ */
++function phutil_nonempty_stringlike($value) {
++  if ($value === null) {
++    return false;
++  }
++
++  if ($value === '') {
++    return false;
++  }
++
++  if (is_string($value)) {
++    return true;
++  }
++
++  if (is_object($value)) {
++    try {
++      $string = phutil_string_cast($value);
++      return phutil_nonempty_string($string);
++    } catch (Exception $ex) {
++      // Continue below.
++    } catch (Throwable $ex) {
++      // Continue below.
++    }
++  }
++
++  throw new InvalidArgumentException(
++    pht(
++      'Call to phutil_nonempty_stringlike() expected a string or stringlike '.
++      'object, got: %s.',
++      phutil_describe_type($value)));
++}
++
++
++/**
++ * Test if a value is a nonempty, scalar value.
++ *
++ * The value "null", the empty string, and objects which have a "__toString()"
++ * method which returns the empty string are empty.
++ *
++ * Other strings, objects with a "__toString()" method which returns a
++ * string other than the empty string, integers, and floats are considered
++ * scalar.
++ *
++ * This method raises an exception if passed any other value.
++ *
++ * @param Value to test.
++ * @return bool True if the parameter is a nonempty, scalar value.
++ */
++function phutil_nonempty_scalar($value) {
++  if ($value === null) {
++    return false;
++  }
++
++  if ($value === '') {
++    return false;
++  }
++
++  if (is_string($value) || is_int($value) || is_float($value)) {
++    return true;
++  }
++
++  if (is_object($value)) {
++    try {
++      $string = phutil_string_cast($value);
++      return phutil_nonempty_string($string);
++    } catch (Exception $ex) {
++      // Continue below.
++    } catch (Throwable $ex) {
++      // Continue below.
++    }
++  }
++
++  throw new InvalidArgumentException(
++    pht(
++      'Call to phutil_nonempty_scalar() expected: a string; or stringlike '.
++      'object; or int; or float. Got: %s.',
++      phutil_describe_type($value)));
++}
++
+diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php
+index edad2d8..dfe8658 100644
+--- a/src/workflow/ArcanistWorkflow.php
++++ b/src/workflow/ArcanistWorkflow.php
+@@ -728,7 +728,7 @@ abstract class ArcanistWorkflow extends Phobject {
+     return $this->workingDirectory;
+   }
+ 
+-  final private function setParentWorkflow($parent_workflow) {
++  /*final*/ private function setParentWorkflow($parent_workflow) {
+     $this->parentWorkflow = $parent_workflow;
+     return $this;
+   }
+@@ -1377,7 +1377,7 @@ abstract class ArcanistWorkflow extends Phobject {
+     ));
+   }
+ 
+-  final private function loadBundleFromConduit(
++  /*final*/ private function loadBundleFromConduit(
+     ConduitClient $conduit,
+     $params) {
+ 
+diff --git a/support/init/init-script.php b/support/init/init-script.php
+index a8c800f..1d1431e 100644
+--- a/support/init/init-script.php
++++ b/support/init/init-script.php
+@@ -15,7 +15,7 @@ function __arcanist_init_script__() {
+     ob_end_clean();
+   }
+ 
+-  error_reporting(E_ALL | E_STRICT);
++  // error_reporting(E_ALL | E_STRICT);
+ 
+   $config_map = array(
+     // Always display script errors. Without this, they may not appear, which is
+-- 
+2.34.1
+