From d25d630fe5dd5bf9c1521bb7b067a8810aca114b Mon Sep 17 00:00:00 2001 From: Valerio Bozzolan Date: Fri, 31 Mar 2023 22:01:56 +0200 Subject: [PATCH] PHP 8.2: fixes for strlen() not accepting NULL anymore, part 1 Summary: This change avoids some unnecessary uses of the strlen() function, actually fixing some deprecation warnings in PHP 8.2. In short, this is the suggested universal replace: -if(strlen($v)) +if(phutil_nonempty_string($v)) And, if you know PHP, this is also another adoptable replace, but only for cases where you are sure that the string "0" is not useful: -if(strlen($v)) +if($v)) As usual the optimal solution depends on the contest. Other similar patches will probably follow. Closes T15222 Ref T15190 Test Plan: - for the first time in my life, with this change, the unit tests are passed in PHP 8.2 - check with your big eyes that there are no obvious typos Reviewers: O1 Blessed Committers, avivey Reviewed By: O1 Blessed Committers, avivey Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno Maniphest Tasks: T15199, T15190, T15222 Differential Revision: https://we.phorge.it/D25104 --- .../check/PhabricatorStorageSetupCheck.php | 8 ++++---- .../engine/PhabricatorS3FileStorageEngine.php | 12 ++++++------ .../files/storage/PhabricatorFile.php | 4 ++-- .../engine/PhabricatorMailEmailEngine.php | 4 ++-- .../client/PhabricatorNotificationServerRef.php | 2 +- .../engine/PhabricatorRepositoryPullEngine.php | 2 +- .../repository/storage/PhabricatorRepository.php | 3 ++- .../cluster/PhabricatorDatabaseRef.php | 2 +- src/infrastructure/env/PhabricatorEnv.php | 16 ++++++++-------- src/infrastructure/log/PhabricatorSSHLog.php | 2 +- 10 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/applications/config/check/PhabricatorStorageSetupCheck.php b/src/applications/config/check/PhabricatorStorageSetupCheck.php index 86aa1f2ebd..75639f2f43 100644 --- a/src/applications/config/check/PhabricatorStorageSetupCheck.php +++ b/src/applications/config/check/PhabricatorStorageSetupCheck.php @@ -151,19 +151,19 @@ final class PhabricatorStorageSetupCheck extends PhabricatorSetupCheck { $how_many = 0; - if (strlen($access_key)) { + if (phutil_nonempty_string($access_key)) { $how_many++; } - if (strlen($secret_key)) { + if (phutil_nonempty_string($secret_key)) { $how_many++; } - if (strlen($region)) { + if (phutil_nonempty_string($region)) { $how_many++; } - if (strlen($endpoint)) { + if (phutil_nonempty_string($endpoint)) { $how_many++; } diff --git a/src/applications/files/engine/PhabricatorS3FileStorageEngine.php b/src/applications/files/engine/PhabricatorS3FileStorageEngine.php index 443a5a9ddc..95cdfc737b 100644 --- a/src/applications/files/engine/PhabricatorS3FileStorageEngine.php +++ b/src/applications/files/engine/PhabricatorS3FileStorageEngine.php @@ -31,11 +31,11 @@ final class PhabricatorS3FileStorageEngine $endpoint = PhabricatorEnv::getEnvConfig('amazon-s3.endpoint'); $region = PhabricatorEnv::getEnvConfig('amazon-s3.region'); - return (strlen($bucket) && - strlen($access_key) && - strlen($secret_key) && - strlen($endpoint) && - strlen($region)); + return phutil_nonempty_string($bucket) && + phutil_nonempty_string($access_key) && + phutil_nonempty_string($secret_key) && + phutil_nonempty_string($endpoint) && + phutil_nonempty_string($region); } @@ -57,7 +57,7 @@ final class PhabricatorS3FileStorageEngine $parts[] = 'phabricator'; $instance_name = PhabricatorEnv::getEnvConfig('cluster.instance'); - if (strlen($instance_name)) { + if (phutil_nonempty_string($instance_name)) { $parts[] = $instance_name; } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index cc1b701dcd..355aa2d215 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -856,7 +856,7 @@ final class PhabricatorFile extends PhabricatorFileDAO // instance identity in the path allows us to distinguish between requests // originating from different instances but served through the same CDN. $instance = PhabricatorEnv::getEnvConfig('cluster.instance'); - if (strlen($instance)) { + if (phutil_nonempty_string($instance)) { $parts[] = '@'.$instance; } @@ -903,7 +903,7 @@ final class PhabricatorFile extends PhabricatorFileDAO $parts[] = 'xform'; $instance = PhabricatorEnv::getEnvConfig('cluster.instance'); - if (strlen($instance)) { + if (phutil_nonempty_string($instance)) { $parts[] = '@'.$instance; } diff --git a/src/applications/metamta/engine/PhabricatorMailEmailEngine.php b/src/applications/metamta/engine/PhabricatorMailEmailEngine.php index 6c9cf1b356..4278aa96a1 100644 --- a/src/applications/metamta/engine/PhabricatorMailEmailEngine.php +++ b/src/applications/metamta/engine/PhabricatorMailEmailEngine.php @@ -507,7 +507,7 @@ final class PhabricatorMailEmailEngine public function newDefaultEmailAddress() { $raw_address = PhabricatorEnv::getEnvConfig('metamta.default-address'); - if (!strlen($raw_address)) { + if (!$raw_address) { $domain = $this->newMailDomain(); $raw_address = "noreply@{$domain}"; } @@ -527,7 +527,7 @@ final class PhabricatorMailEmailEngine private function newMailDomain() { $domain = PhabricatorEnv::getEnvConfig('metamta.reply-handler-domain'); - if (strlen($domain)) { + if ($domain) { return $domain; } diff --git a/src/applications/notification/client/PhabricatorNotificationServerRef.php b/src/applications/notification/client/PhabricatorNotificationServerRef.php index 714ad5f5d7..c5b0f2f8aa 100644 --- a/src/applications/notification/client/PhabricatorNotificationServerRef.php +++ b/src/applications/notification/client/PhabricatorNotificationServerRef.php @@ -152,7 +152,7 @@ final class PhabricatorNotificationServerRef ->setPath($full_path); $instance = PhabricatorEnv::getEnvConfig('cluster.instance'); - if (strlen($instance)) { + if (phutil_nonempty_string($instance)) { $uri->replaceQueryParam('instance', $instance); } diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index 12aee9bc51..5bcf4554fd 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -238,7 +238,7 @@ final class PhabricatorRepositoryPullEngine $identifier = $repository->getPHID(); $instance = PhabricatorEnv::getEnvConfig('cluster.instance'); - if (strlen($instance)) { + if (phutil_nonempty_string($instance)) { $identifier = "{$identifier}:{$instance}"; } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index c68eb22668..5c7c9ff33d 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -2480,7 +2480,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $has_https = false; } - $has_ssh = (bool)strlen(PhabricatorEnv::getEnvConfig('phd.user')); + $phd_user = PhabricatorEnv::getEnvConfig('phd.user'); + $has_ssh = phutil_nonempty_string($phd_user); $protocol_map = array( PhabricatorRepositoryURI::BUILTIN_PROTOCOL_SSH => $has_ssh, diff --git a/src/infrastructure/cluster/PhabricatorDatabaseRef.php b/src/infrastructure/cluster/PhabricatorDatabaseRef.php index 1eb232ad86..e526eb1639 100644 --- a/src/infrastructure/cluster/PhabricatorDatabaseRef.php +++ b/src/infrastructure/cluster/PhabricatorDatabaseRef.php @@ -229,7 +229,7 @@ final class PhabricatorDatabaseRef $host = $this->getHost(); $port = $this->getPort(); - if (strlen($port)) { + if ($port) { return "{$host}:{$port}"; } diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index ca478ed0a5..617fb57af9 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -125,7 +125,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'); - if (strlen($instance)) { + if (phutil_nonempty_string($instance)) { putenv('PHABRICATOR_INSTANCE='.$instance); $_ENV['PHABRICATOR_INSTANCE'] = $instance; } @@ -432,7 +432,7 @@ final class PhabricatorEnv extends Phobject { $uri = new PhutilURI($raw_uri); $host = $uri->getDomain(); - if (!strlen($host)) { + if (!phutil_nonempty_string($host)) { return false; } @@ -455,7 +455,7 @@ final class PhabricatorEnv extends Phobject { $self_map = array(); foreach ($self_uris as $self_uri) { $host = id(new PhutilURI($self_uri))->getDomain(); - if (!strlen($host)) { + if (!phutil_nonempty_string($host)) { continue; } @@ -661,7 +661,7 @@ final class PhabricatorEnv extends Phobject { public static function isValidLocalURIForLink($uri) { $uri = (string)$uri; - if (!strlen($uri)) { + if (!phutil_nonempty_string($uri)) { return false; } @@ -726,7 +726,7 @@ final class PhabricatorEnv extends Phobject { $uri = new PhutilURI($raw_uri); $proto = $uri->getProtocol(); - if (!strlen($proto)) { + if (!$proto) { throw new Exception( pht( 'URI "%s" is not a valid linkable resource. A valid linkable '. @@ -745,7 +745,7 @@ final class PhabricatorEnv extends Phobject { } $domain = $uri->getDomain(); - if (!strlen($domain)) { + if (!$domain) { throw new Exception( pht( 'URI "%s" is not a valid linkable resource. A valid linkable '. @@ -793,7 +793,7 @@ final class PhabricatorEnv extends Phobject { $uri = new PhutilURI($raw_uri); $proto = $uri->getProtocol(); - if (!strlen($proto)) { + if (!$proto) { throw new Exception( pht( 'URI "%s" is not a valid fetchable resource. A valid fetchable '. @@ -812,7 +812,7 @@ final class PhabricatorEnv extends Phobject { } $domain = $uri->getDomain(); - if (!strlen($domain)) { + if (!$domain) { throw new Exception( pht( 'URI "%s" is not a valid fetchable resource. A valid fetchable '. diff --git a/src/infrastructure/log/PhabricatorSSHLog.php b/src/infrastructure/log/PhabricatorSSHLog.php index de4c270f72..e3c15de6bf 100644 --- a/src/infrastructure/log/PhabricatorSSHLog.php +++ b/src/infrastructure/log/PhabricatorSSHLog.php @@ -24,7 +24,7 @@ final class PhabricatorSSHLog extends Phobject { ); $sudo_user = PhabricatorEnv::getEnvConfig('phd.user'); - if (strlen($sudo_user)) { + if (phutil_nonempty_string($sudo_user)) { $data['S'] = $sudo_user; }