Addressing some PHP 8 incompatibilities

Summary:
Starting with a new instance running PHP 8.2, address all exceptions that come up through some basic browsing/usage.

For `strlen(null)` issues I generally tried to resolve if the value should be non-null at the point of issue, and attempt to address at earlier call-site. There were not many of these that I could determine. In the rest of those cases I would replace with a null-and-strlen check, or use `phutil_nonempty_string` if I was certain the value was a string and it was more convenient.

Hitting all code-paths is challenging, so I would search for `strlen` within radius of files I was modifying and evaluate to address those uses in the same manner.

Notes:
- `AphrontRequest::getStr` only ever returns a string, and is safe to use `phutil_nonempty_string`.
- `PhabricatorEnv::getEnvConfig` can return non-string things so any values coming from there should never use `phutil_nonempty_string`.
- `AphrontRequest::getHTTPHeader` indicates it could return wild so `phutil_nonempty_string` should not be used.
- `AphrontRequest::getURIData` isn't clear if it could return non-string data, so never use `phutil_nonempty_string`.

Refs T13588

Test Plan: I'm running an instance on 8.2 and went through the basic setup/installation, startup and usage, including setup issues and configurations/settings.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: aklapper, Korvin, epriestley

Maniphest Tasks: T13588

Differential Revision: https://secure.phabricator.com/D21862

Author: neandrake

.gitignore

@@ -36,6 +36,8 @@
 
 # NPM local packages
 /support/aphlict/server/node_modules/
+/support/aphlict/server/package.json
+/support/aphlict/server/package-lock.json
 
 # Places for users to add custom resources.
 /resources/cows/custom/*

src/aphront/AphrontRequest.php

@@ -66,7 +66,7 @@ public function getURILineRange($key, $limit) {
   }
 
   public static function parseURILineRange($range, $limit) {
-    if (!strlen($range)) {
+    if ($range === null || !strlen($range)) {
       return null;
     }
 
@@ -448,11 +448,10 @@ public function setCookiePrefix($prefix) {
   }
 
   private function getPrefixedCookieName($name) {
-    if (strlen($this->cookiePrefix)) {
+    if ($this->cookiePrefix !== null && strlen($this->cookiePrefix)) {
       return $this->cookiePrefix.'_'.$name;
-    } else {
-      return $name;
     }
+    return $name;
   }
 
   public function getCookie($name, $default = null) {
@@ -499,7 +498,7 @@ private function getCookieDomainURI() {
     // domain is. This makes setup easier, and we'll tell administrators to
     // configure a base domain during the setup process.
     $base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
-    if (!strlen($base_uri)) {
+    if ($base_uri === null || !strlen($base_uri)) {
       return new PhutilURI('http://'.$host.'/');
     }
 
@@ -956,7 +955,7 @@ public function updateEphemeralCookies() {
     $submit_cookie = PhabricatorCookies::COOKIE_SUBMIT;
 
     $submit_key = $this->getCookie($submit_cookie);
-    if (strlen($submit_key)) {
+    if ($submit_key !== null && strlen($submit_key)) {
       $this->clearCookie($submit_cookie);
       $this->submitKey = $submit_key;
     }

src/aphront/response/AphrontFileResponse.php

@@ -19,7 +19,7 @@ public function addAllowOrigin($origin) {
   }
 
   public function setDownload($download) {
-    if (!strlen($download)) {
+    if ($download === null || !strlen($download)) {
       $download = 'untitled';
     }
     $this->download = $download;
@@ -113,7 +113,8 @@ public function getHeaders() {
       $headers[] = array('Content-Length', $content_len);
     }
 
-    if (strlen($this->getDownload())) {
+    $download = $this->getDownload();
+    if ($download !== null && strlen($download)) {
       $headers[] = array('X-Download-Options', 'noopen');
 
       $filename = $this->getDownload();
@@ -150,7 +151,7 @@ public function parseHTTPRange($range) {
       $begin = (int)$matches[1];
 
       // The "Range" may be "200-299" or "200-", meaning "until end of file".
-      if (strlen($matches[2])) {
+      if ($matches[2] !== null && strlen($matches[2])) {
         $range_end = (int)$matches[2];
         $end = $range_end + 1;
       } else {

src/aphront/response/AphrontWebpageResponse.php

@@ -21,7 +21,7 @@ public function getUnexpectedOutput() {
 
   public function buildResponseString() {
     $unexpected_output = $this->getUnexpectedOutput();
-    if (strlen($unexpected_output)) {
+    if ($unexpected_output !== null && strlen($unexpected_output)) {
       $style = array(
         'background: linear-gradient(180deg, #eeddff, #ddbbff);',
         'white-space: pre-wrap;',

src/applications/auth/constants/PhabricatorCookies.php

@@ -89,7 +89,7 @@ public static function setClientIDCookie(AphrontRequest $request) {
     // temporary and clearing it when users log out.
 
     $value = $request->getCookie(self::COOKIE_CLIENTID);
-    if (!strlen($value)) {
+    if ($value === null || !strlen($value)) {
       $request->setTemporaryCookie(
         self::COOKIE_CLIENTID,
         Filesystem::readRandomCharacters(16));

src/applications/auth/controller/PhabricatorAuthController.php

@@ -282,7 +282,7 @@ final protected function newCustomStartMessage() {
       $viewer,
       PhabricatorAuthLoginMessageType::MESSAGEKEY);
 
-    if (!strlen($text)) {
+    if ($text === null || !strlen($text)) {
       return null;
     }
 

src/applications/auth/controller/PhabricatorAuthRegisterController.php

@@ -18,7 +18,7 @@ public function handleRequest(AphrontRequest $request) {
     $invite = $this->loadInvite();
 
     $is_setup = false;
-    if (strlen($account_key)) {
+    if ($account_key !== null && strlen($account_key)) {
       $result = $this->loadAccountForRegistrationOrLinking($account_key);
       list($account, $provider, $response) = $result;
       $is_default = false;
@@ -244,9 +244,9 @@ public function handleRequest(AphrontRequest $request) {
 
     $require_real_name = PhabricatorEnv::getEnvConfig('user.require-real-name');
 
-    $e_username = strlen($value_username) ? null : true;
+    $e_username = phutil_nonempty_string($value_username) ? null : true;
     $e_realname = $require_real_name ? true : null;
-    $e_email = strlen($value_email) ? null : true;
+    $e_email = phutil_nonempty_string($value_email) ? null : true;
     $e_password = true;
     $e_captcha = true;
 
@@ -288,7 +288,7 @@ public function handleRequest(AphrontRequest $request) {
 
       if ($can_edit_username) {
         $value_username = $request->getStr('username');
-        if (!strlen($value_username)) {
+        if (!phutil_nonempty_string($value_username)) {
           $e_username = pht('Required');
           $errors[] = pht('Username is required.');
         } else if (!PhabricatorUser::validateUsername($value_username)) {
@@ -323,7 +323,7 @@ public function handleRequest(AphrontRequest $request) {
 
       if ($can_edit_email) {
         $value_email = $request->getStr('email');
-        if (!strlen($value_email)) {
+        if (!phutil_nonempty_string($value_email)) {
           $e_email = pht('Required');
           $errors[] = pht('Email is required.');
         } else if (!PhabricatorUserEmail::isValidAddress($value_email)) {
@@ -339,7 +339,7 @@ public function handleRequest(AphrontRequest $request) {
 
       if ($can_edit_realname) {
         $value_realname = $request->getStr('realName');
-        if (!strlen($value_realname) && $require_real_name) {
+        if (!phutil_nonempty_string($value_realname) && $require_real_name) {
           $e_realname = pht('Required');
           $errors[] = pht('Real name is required.');
         } else {

src/applications/auth/controller/PhabricatorAuthStartController.php

@@ -31,7 +31,7 @@ public function handleRequest(AphrontRequest $request) {
     $session_token = $request->getCookie(PhabricatorCookies::COOKIE_SESSION);
     $did_clear = $request->getStr('cleared');
 
-    if (strlen($session_token)) {
+    if ($session_token !== null && strlen($session_token)) {
       $kind = PhabricatorAuthSessionEngine::getSessionKindFromToken(
         $session_token);
       switch ($kind) {
@@ -98,7 +98,7 @@ public function handleRequest(AphrontRequest $request) {
     }
 
     $next_uri = $request->getStr('next');
-    if (!strlen($next_uri)) {
+    if (phutil_nonempty_string($next_uri)) {
       if ($this->getDelegatingController()) {
         // Only set a next URI from the request path if this controller was
         // delegated to, which happens when a user tries to view a page which
@@ -112,7 +112,7 @@ public function handleRequest(AphrontRequest $request) {
     }
 
     if (!$request->isFormPost()) {
-      if (strlen($next_uri)) {
+      if (phutil_nonempty_string($next_uri)) {
         PhabricatorCookies::setNextURICookie($request, $next_uri);
       }
       PhabricatorCookies::setClientIDCookie($request);
@@ -226,7 +226,7 @@ private function processAjaxRequest() {
 
     $via_header = AphrontRequest::getViaHeaderName();
     $via_uri = AphrontRequest::getHTTPHeader($via_header);
-    if (strlen($via_uri)) {
+    if ($via_uri !== null && strlen($via_uri)) {
       PhabricatorCookies::setNextURICookie($request, $via_uri, $force = true);
     }
 

src/applications/auth/password/PhabricatorAuthPasswordException.php

@@ -4,7 +4,7 @@ final class PhabricatorAuthPasswordException
   extends Exception {
 
   private $passwordError;
-  private $confirmErorr;
+  private $confirmError;
 
   public function __construct(
     $message,

src/applications/base/controller/PhabricatorController.php

@@ -74,7 +74,7 @@ public function willBeginExecution() {
       $session_engine = new PhabricatorAuthSessionEngine();
 
       $phsid = $request->getCookie(PhabricatorCookies::COOKIE_SESSION);
-      if (strlen($phsid)) {
+      if ($phsid !== null && strlen($phsid)) {
         $session_user = $session_engine->loadUserForSession(
           PhabricatorAuthSession::TYPE_WEB,
           $phsid);

src/applications/celerity/controller/CelerityResourceController.php

@@ -113,7 +113,7 @@ protected function serveResource(array $spec) {
 
     $range = AphrontRequest::getHTTPHeader('Range');
 
-    if (strlen($range)) {
+    if ($range !== null && strlen($range)) {
       $response->setContentLength(strlen($data));
 
       list($range_begin, $range_end) = $response->parseHTTPRange($range);

src/applications/config/check/PhabricatorBaseURISetupCheck.php

@@ -11,7 +11,7 @@ protected function executeChecks() {
 
     $host_header = AphrontRequest::getHTTPHeader('Host');
     if (strpos($host_header, '.') === false) {
-      if (!strlen(trim($host_header))) {
+      if ($host_header === null || !strlen(trim($host_header))) {
         $name = pht('No "Host" Header');
         $summary = pht('No "Host" header present in request.');
         $message = pht(

src/applications/config/check/PhabricatorDaemonsSetupCheck.php

@@ -53,7 +53,7 @@ protected function executeChecks() {
     }
 
     $expect_user = PhabricatorEnv::getEnvConfig('phd.user');
-    if (strlen($expect_user)) {
+    if ($expect_user !== null && strlen($expect_user)) {
 
       try {
         $all_daemons = id(new PhabricatorDaemonLogQuery())

src/applications/config/check/PhabricatorPHPPreflightSetupCheck.php

@@ -124,7 +124,7 @@ protected function executeChecks() {
     }
 
     $open_basedir = ini_get('open_basedir');
-    if (strlen($open_basedir)) {
+    if ($open_basedir !== null && strlen($open_basedir)) {
       // If `open_basedir` is set, just fatal. It's technically possible for
       // us to run with certain values of `open_basedir`, but: we can only
       // raise fatal errors from preflight steps, so we'd have to do this check

src/applications/config/check/PhabricatorSetupCheck.php

@@ -122,7 +122,7 @@ final public static function getOpenSetupIssueKeysFromDatabase() {
     $db_cache = new PhabricatorKeyValueDatabaseCache();
     try {
       $value = $db_cache->getKey('phabricator.setup.issue-keys');
-      if (!strlen($value)) {
+      if ($value === null || !strlen($value)) {
         return null;
       }
       return phutil_json_decode($value);

src/applications/config/check/PhabricatorStorageSetupCheck.php

@@ -151,19 +151,19 @@ private function checkS3() {
 
     $how_many = 0;
 
-    if (strlen($access_key)) {
+    if ($access_key !== null && strlen($access_key)) {
       $how_many++;
     }
 
-    if (strlen($secret_key)) {
+    if ($secret_key !== null && strlen($secret_key)) {
       $how_many++;
     }
 
-    if (strlen($region)) {
+    if ($region !== null && strlen($region)) {
       $how_many++;
     }
 
-    if (strlen($endpoint)) {
+    if ($endpoint !== null && strlen($endpoint)) {
       $how_many++;
     }
 

src/applications/config/check/PhabricatorWebServerSetupCheck.php

@@ -23,7 +23,7 @@ protected function executeChecks() {
     }
 
     $base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
-    if (!strlen($base_uri)) {
+    if ($base_uri === null || !strlen($base_uri)) {
       // If `phabricator.base-uri` is not set then we can't really do
       // anything.
       return;

src/applications/config/controller/PhabricatorConfigConsoleController.php

@@ -85,14 +85,14 @@ public function newLibraryVersionTable() {
     $rows = array();
     foreach ($versions as $name => $info) {
       $branchpoint = $info['branchpoint'];
-      if (strlen($branchpoint)) {
+      if ($branchpoint !== null && strlen($branchpoint)) {
         $branchpoint = substr($branchpoint, 0, 12);
       } else {
         $branchpoint = null;
       }
 
       $version = $info['hash'];
-      if (strlen($version)) {
+      if ($version !== null && strlen($version)) {
         $version = substr($version, 0, 12);
       } else {
         $version = pht('Unknown');

src/applications/config/controller/services/PhabricatorConfigDatabaseStatusController.php

@@ -837,7 +837,7 @@ private function getURI(array $properties) {
 
     $parts = array();
     foreach ($properties as $key => $property) {
-      if (!strlen($property)) {
+      if ($property === null || !strlen($property)) {
         continue;
       }
 

src/applications/config/controller/settings/PhabricatorConfigSettingsListController.php

@@ -7,7 +7,7 @@ public function handleRequest(AphrontRequest $request) {
     $viewer = $request->getViewer();
 
     $filter = $request->getURIData('filter');
-    if (!strlen($filter)) {
+    if ($filter === null || !strlen($filter)) {
       $filter = 'settings';
     }
 

src/applications/console/controller/DarkConsoleController.php

@@ -26,15 +26,15 @@ public function handleRequest(AphrontRequest $request) {
     }
 
     $visible = $request->getStr('visible');
-    if (strlen($visible)) {
+    if (phutil_nonempty_string($visible)) {
       $this->writeDarkConsoleSetting(
         PhabricatorDarkConsoleVisibleSetting::SETTINGKEY,
         (int)$visible);
       return $response;
     }
 
     $tab = $request->getStr('tab');
-    if (strlen($tab)) {
+    if (phutil_nonempty_string($tab)) {
       $this->writeDarkConsoleSetting(
         PhabricatorDarkConsoleTabSetting::SETTINGKEY,
         $tab);

src/applications/console/core/DarkConsoleCore.php

@@ -114,6 +114,9 @@ public function render(AphrontRequest $request) {
    * need to convert it to UTF-8.
    */
   private function sanitizeForJSON($data) {
+    if ($data === null) {
+      return '<null>';
+    }
     if (is_object($data)) {
       return '<object:'.get_class($data).'>';
     } else if (is_array($data)) {

src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php

@@ -55,7 +55,7 @@ protected function getGitResult(ConduitAPIRequest $request) {
     $limit = $request->getValue('limit');
 
     if (strlen($against_hash)) {
-      $commit_range = "${against_hash}..${commit_hash}";
+      $commit_range = "{$against_hash}..{$commit_hash}";
     } else {
       $commit_range = $commit_hash;
     }

src/applications/diffusion/controller/DiffusionController.php

@@ -97,19 +97,19 @@ protected function getRepositoryIdentifierFromRequest(
     AphrontRequest $request) {
 
     $short_name = $request->getURIData('repositoryShortName');
-    if (strlen($short_name)) {
+    if ($short_name !== null && strlen($short_name)) {
       // If the short name ends in ".git", ignore it.
       $short_name = preg_replace('/\\.git\z/', '', $short_name);
       return $short_name;
     }
 
     $identifier = $request->getURIData('repositoryCallsign');
-    if (strlen($identifier)) {
+    if ($identifier !== null && strlen($identifier)) {
       return $identifier;
     }
 
     $id = $request->getURIData('repositoryID');
-    if (strlen($id)) {
+    if ($id !== null && strlen($id)) {
       return (int)$id;
     }
 

src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php

@@ -183,11 +183,11 @@ private function composeImage($color, $image, $border) {
       'image/png');
   }
 
-  private static function rgba2gd($rgba) {
-    $r = $rgba[0];
-    $g = $rgba[1];
-    $b = $rgba[2];
-    $a = $rgba[3];
+  private static function rgba2gd(array $rgba) {
+    $r = (int)$rgba[0];
+    $g = (int)$rgba[1];
+    $b = (int)$rgba[2];
+    $a = (int)$rgba[3];
     $a = (1 - $a) * 255;
     return ($a << 24) | ($r << 16) | ($g << 8) | $b;
   }