NOTE: Parent Task: {T15190}
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:
```lang=php
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:
```lang=diff
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:
```lang=diff
public function newDefaultEmailAddress() {
$test = PhabricatorEnv::getEnvConfig('test');
- if (strlen($test)) {
+ if ($test !== null && is_string( $text) && $text !== '') {
return $test;
```
== Introducing getEnvConfigString() ==
So, we could introduce this simple wrapper method:
```lang=php
public static function getEnvConfigString($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:
```lang=diff
- $test = PhabricatorEnv::getEnvConfig('test');
+ $test = PhabricatorEnv::getEnvConfigString('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:
```lang=diff
- $test = PhabricatorEnv::getEnvConfig('test');
+ $test = PhabricatorEnv::getEnvConfigString('test');
- if (strlen($test)) {
+ if ($test) {
```
IMPORTANT: It is not to say that we should introduce this function to then simply use `if($value)` from now on. I'm saying that, we can do that, for the cases we KNOW that it has sense, and, for all the normal cases, we can just use `if($value !== '')`.
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:
| Approach | When you should use it / Pre-conditions |
|------------------|-----------------------------------------------------------|
| `if($var !== '')`| 100% correct method to check if the value is non-empty. |
| `if($var)` | Shorter, still 100% correct, and cute, **ONLY** if the value `0` is NEVER good for you. For example, if `$var` contains an email address, this version is OK. |
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.
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)