[PATCH 0/3] Fixes for basic PHP 8 functionality

From baf002304129fcfd8af8d6d1986310bd93b167ce Mon Sep 17 00:00:00 2001
From: Jessica Clarke <jrtc27@jrtc27.com>
Date: Sat, 9 Jan 2021 03:57:56 +0000
Subject: [PATCH 0/3] Fixes for basic PHP 8 functionality

This patch series is enough to get basic arc functionality to work with
PHP 8 (arc diff works, at least up until writing the review metadata, I
have yet to have another review to actually submit somewhere so it could
still be broken after that point). More may follow if I discover other
broken things.

Since I do not have an account on secure.phabricator.com this about the
only vaguely-sensible place for me to post these patches. I was going to
upload them but apparently only images are allowed so instead the best
you'll get is them concatenated inline below. If given an account I am
happy to post them for proper code review.

I have signed the CLA.

Jessica Clarke (3):
  Remove final from private functions for PHP 8 compatibility
  Fix PHP 8 deprecation warning in __arcanist_init_script__
  Fix error handler on PHP 8

 src/error/PhutilErrorHandler.php          |  5 ++---
 src/error/PhutilErrorTrap.php             |  3 +--
 src/lint/engine/ArcanistLintEngine.php    |  4 ++--
 src/lint/linter/ArcanistLinter.php        |  2 +-
 src/unit/engine/phutil/PhutilTestCase.php | 16 ++++++++--------
 src/utils/AbstractDirectedGraph.php       |  2 +-
 src/workflow/ArcanistWorkflow.php         |  4 ++--
 support/init/init-script.php              |  3 ++-
 8 files changed, 19 insertions(+), 20 deletions(-)

-- 
2.29.2
From 9a3a21bfd7a88fd00a568b0a93f0cbae51d13ec5 Mon Sep 17 00:00:00 2001
From: Jessica Clarke <jrtc27@jrtc27.com>
Date: Sat, 9 Jan 2021 03:42:05 +0000
Subject: [PATCH 1/3] Remove final from private functions for PHP 8
 compatibility

This combination does not make sense and PHP 8 errors with:

```
Private methods cannot be final as they are never overridden by other classes
```

Thus remove the redundant final from all such functions.
---
 src/lint/engine/ArcanistLintEngine.php    |  4 ++--
 src/lint/linter/ArcanistLinter.php        |  2 +-
 src/unit/engine/phutil/PhutilTestCase.php | 16 ++++++++--------
 src/utils/AbstractDirectedGraph.php       |  2 +-
 src/workflow/ArcanistWorkflow.php         |  4 ++--
 5 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/lint/engine/ArcanistLintEngine.php b/src/lint/engine/ArcanistLintEngine.php
index f737fcd4..add11d15 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(
+  private function shouldUseCache(
     $cache_granularity,
     $repository_version) {
 
@@ -313,7 +313,7 @@ abstract class ArcanistLintEngine extends Phobject {
     return $this;
   }
 
-  final private function isRelevantMessage(ArcanistLintMessage $message) {
+  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 afb8cbe3..85a94453 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) {
+  private function filterPaths(array $paths) {
     $engine = $this->getEngine();
 
     $keep = array();
diff --git a/src/unit/engine/phutil/PhutilTestCase.php b/src/unit/engine/phutil/PhutilTestCase.php
index fddb0681..efa946d9 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) {
+  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) {
+  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) {
+  private function skipTest($reason) {
     $this->resultTest(ArcanistUnitTestResult::RESULT_SKIP, $reason);
   }
 
 
-  final private function resultTest($test_result, $reason) {
+  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() {
+  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() {
+  private function endCoverage() {
     if (!$this->enableCoverage) {
       return;
     }
@@ -618,7 +618,7 @@ abstract class PhutilTestCase extends Phobject {
     return $coverage;
   }
 
-  final private function assertCoverageAvailable() {
+  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() {
+  private static function getCallerInfo() {
     $callee = array();
     $caller = array();
     $seen = false;
diff --git a/src/utils/AbstractDirectedGraph.php b/src/utils/AbstractDirectedGraph.php
index 509f0f5f..24611c14 100644
--- a/src/utils/AbstractDirectedGraph.php
+++ b/src/utils/AbstractDirectedGraph.php
@@ -318,7 +318,7 @@ abstract class AbstractDirectedGraph extends Phobject {
    *                      which cycle.
    * @task cycle
    */
-  final private function performCycleDetection($node, array $visited) {
+  private function performCycleDetection($node, array $visited) {
     $visited[$node] = true;
     foreach ($this->knownNodes[$node] as $edge) {
       if (isset($visited[$edge])) {
diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php
index edad2d8d..9d6adb91 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) {
+  private function setParentWorkflow($parent_workflow) {
     $this->parentWorkflow = $parent_workflow;
     return $this;
   }
@@ -1377,7 +1377,7 @@ abstract class ArcanistWorkflow extends Phobject {
     ));
   }
 
-  final private function loadBundleFromConduit(
+  private function loadBundleFromConduit(
     ConduitClient $conduit,
     $params) {
 
-- 
2.29.2
From f78c184a6025c05de4c3a0e2603a834d074e8d09 Mon Sep 17 00:00:00 2001
From: Jessica Clarke <jrtc27@jrtc27.com>
Date: Sat, 9 Jan 2021 03:44:21 +0000
Subject: [PATCH 2/3] Fix PHP 8 deprecation warning in __arcanist_init_script__

As of PHP 8, the XML entity loader is disabled by default and the
libxml_disable_entity_loader function is deprecated. Thus only call it
for versions of PHP lower than 8.
---
 support/init/init-script.php | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/support/init/init-script.php b/support/init/init-script.php
index 2b742074..fdbab39a 100644
--- a/support/init/init-script.php
+++ b/support/init/init-script.php
@@ -94,7 +94,8 @@ function __arcanist_init_script__() {
   )));
 
   // Disable the insanely dangerous XML entity loader by default.
-  if (function_exists('libxml_disable_entity_loader')) {
+  // PHP 8 deprecates this function and disables this by default.
+  if (function_exists('libxml_disable_entity_loader') && \PHP_VERSION_ID < 80000) {
     libxml_disable_entity_loader(true);
   }
 
-- 
2.29.2
From baf002304129fcfd8af8d6d1986310bd93b167ce Mon Sep 17 00:00:00 2001
From: Jessica Clarke <jrtc27@jrtc27.com>
Date: Sat, 9 Jan 2021 03:54:38 +0000
Subject: [PATCH 3/3] Fix error handler on PHP 8

PHP 7.2.0 deprecated the 5th parameter and PHP 8 removed it, so stop
using it and provide a default value to avoid erroring with:

```
Too few arguments to function PhutilErrorHandler::handleError(), 4 passed and exactly 5 expected
```
---
 src/error/PhutilErrorHandler.php | 5 ++---
 src/error/PhutilErrorTrap.php    | 3 +--
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/error/PhutilErrorHandler.php b/src/error/PhutilErrorHandler.php
index deb6ee16..95af88b6 100644
--- a/src/error/PhutilErrorHandler.php
+++ b/src/error/PhutilErrorHandler.php
@@ -180,10 +180,10 @@ 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 = null) {
 
     foreach (self::$traps as $trap) {
-      $trap->addError($num, $str, $file, $line, $ctx);
+      $trap->addError($num, $str, $file, $line);
     }
 
     if ((error_reporting() & $num) == 0) {
@@ -214,7 +214,6 @@ final class PhutilErrorHandler extends Phobject {
         array(
           'file'       => $file,
           'line'       => $line,
-          'context'    => $ctx,
           'error_code' => $num,
           'trace'      => $trace,
         ));
diff --git a/src/error/PhutilErrorTrap.php b/src/error/PhutilErrorTrap.php
index 94d291d0..252a37f9 100644
--- a/src/error/PhutilErrorTrap.php
+++ b/src/error/PhutilErrorTrap.php
@@ -41,13 +41,12 @@ final class PhutilErrorTrap extends Phobject {
   private $destroyed;
   private $errors = array();
 
-  public function addError($num, $str, $file, $line, $ctx) {
+  public function addError($num, $str, $file, $line) {
     $this->errors[] = array(
       'num' => $num,
       'str' => $str,
       'file' => $file,
       'line' => $line,
-      'ctx' => $ctx,
     );
     return $this;
   }
-- 
2.29.2

Are jrtc27 and the author address in your commits your preferred username and email address for an account on secure.phabricator.com?

Yes, thanks.

Cool, you should have an email with login instructions – submit your patches to secure and I’ll follow up there. Let me know if you run into issues. Thanks!