We call so much time strlen().

Why? One of the reasons is that it's used to check PhabricatorEnv::getEnvConfig() - whenever it returns an empty value or not.

Why? Because the method is_empty() is fondamentally broken - it thinks that the string "0" is empty, for example. So we use strlen().

Why? Because the method PhabricatorEnv::getEnvConfig() returns a mixed value. The problem is, most of our checks are something like this:

public function newDefaultEmailAddress() {
  $test = PhabricatorEnv::getEnvConfig('test');
  if (strlen($test)) {

Note that checking strlen() had sense to both assume that:

• it was a string
• it is not an empty string

Note that, we use strlen() to just check if it's zero, but strlen() is slower than is_empty() - since strlen() counts every single character, while is_empty() just tells whether it is empty or not.

Why a new method could be useful

Note that, without changing anything else, the natural change should be this one now, that is literally the fix:

 public function newDefaultEmailAddress() {
   $test = PhabricatorEnv::getEnvConfig('test');
-  if (strlen($test)) {
+  if ($test != null && strlen($test)) {
     return $test;

Why? Why do we count for the length of the string? We just need to verify if it's empty, after all.

Probably, we do that, since this is the natural equivalent, but it's too much long:

 public function newDefaultEmailAddress() {
   $test = PhabricatorEnv::getEnvConfig('test');
-  if (strlen($test)) {
+  if (is_string($test) && $test !== '') {
     return $test;

Introducing getEnvConfigString()

So, we could introduce this simple wrapper method:

public static function getEnvConfigStr($key) {
  return (string) self::getEnvConfig($key);
}

The above method is very efficient since:

• in the base case, any long string is just returned as-is
• the NULL value is just converted as '' (very quick operation)
• any other weird nonsense type is just returned as an empty string (very quick operation)

So, introducing getEnvConfigString(), every use can eliminate two tests, since they can assume that the return value is a string, and it's never NULL. So the return value can be just an empty string, or a longer string.

So, this would be the average usage:

-  $test = PhabricatorEnv::getEnvConfig('test');
+  $test = PhabricatorEnv::getEnvConfigStr('test');

-  if (strlen($test)) {
+  if ($test !== '') {

Also, for configurations that NEVER returns zero as a meaningful value (like configurations returning an email address or not, for example), we can finally rely on PHP's internal behaviors to simply do this:

-  $test = PhabricatorEnv::getEnvConfig('test');
+  $test = PhabricatorEnv::getEnvConfigStr('test');

-  if (strlen($test)) {
+  if ($test) {

So.

What should be used with PhabricatorEnv::getEnvConfigString() to check if your configuration value is non empty?

Pre-conditions: now we are SURE that $var is a string and it's not NULL:

The above are the two feasible checks.

(All other checks like == or != or is_empty or isset could be just confusing and could be avoided here. Your $var always is a string, the point is to check whenever it's empty or not. For example == and != should not be used if you are sure that you are comparing two identical things. Also, is_empty and isset are too much high level: you have a string, not a generic stuff.

Truth table:

In short this method would simplify fix of PHP 8.2 support and also:

• it's so much readable than seeing strlen used in an if that also converts it to a truly/falsy basing on the fact that 0 is falsy
• it's more efficient (we don't count anymore the length of the string to just check whenever it's empty - we save CPU cycles - we save energy - we save the environment - Greta Thunberg is happier)