Page MenuHomePhorge

PHP 8.2: fixes for strlen() not accepting NULL anymore
AbandonedPublic

Authored by valerio.bozzolan on Mar 26 2023, 14:14.

Details

Summary

This change introduces this very stupid wrapper:

PhabricatorEnv::getEnvConfigString()

This method always returns a string. So, if the value is empty,
the return value is just an empty string. This is useful to simplify
most of our needed checks.

https://we.phorge.it/T15199

So, to verify that a value is not empty, now you can of course:

$value = PhabricatorEnv::getEnvConfigString('foo');
if($value !== '') {
  ...
}

Or, if you are 100% sure that the value '0' is not useful
at all and can be discarded, you can also love this version
that should be adopted only by true PHP experts and with a
master in type juggling from string to boolean:

$value = PhabricatorEnv::getEnvConfigString('foo');
if($value) {
   ...
}

In any case - never use a PHP feature unless you know PHP. This
can be not surprising, but it's really something deep and to
keep in mind to avoid over-complications caused by PHP.

In short, with this method is very simple to fix in an elegant and
more efficient way some fixes for PHP 8.2 - as you can see.

Note that it also avoids to use @strlen() since it is not efficient,
because it spawns a warning that it's then discarded. Not good for you.

At the moment for example I was able to run some Arcanist commands
without any warning related to strlen(). Wow.

Closes T15199
Ref T15190

Test Plan
  • I checked with my big eyes every line

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25097
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 197
Build 197: arc lint + arc unit

Event Timeline

use PHP 7.4 to make lint happy

src/applications/files/engine/PhabricatorS3FileStorageEngine.php
38

↑ I also removed the parentheses since they were totally usefulness and just an additional risk to make typos in the last line.

src/applications/metamta/engine/PhabricatorMailEmailEngine.php
509

↑ Note that this check is correct, since zero will never be a valid email so we can just have a nice if, clean and simple.

529

↑ Note that this check is correct, since zero will never be a valid domain so we can just have a nice if, clean and simple.

try to make the diff even more simple

src/applications/repository/storage/PhabricatorRepository.php
2483

Using strlen() to assure the value as a string, then getting its char count, then casting to bool was probably too much.

src/infrastructure/cluster/PhabricatorDatabaseRef.php
232

This short syntax is appropriate for this case where the subject must be a valid port, and zero is not a valid port.

src/applications/repository/storage/PhabricatorRepository.php
2483

I mean, it was definitely too much. I have no doubts about that. It was just to say.

ollehar added inline comments.
src/infrastructure/env/PhabricatorEnv.php
390

Shouldn't type come before variable name?

https://docs.phpdoc.org/3.0/guide/references/phpdoc/tags/param.html

Maybe Phorge is using another convention?

src/infrastructure/env/PhabricatorEnv.php
390

Nice thanks, my fault

fix PHPDoc thanks to suggestion of user @ollehar

I think this change is reasonable, but I think that Evan will fix this in this way:

-if(strlen($s))
+if(phutil_nonempty_string($s))

If you agree, I can abandon this proposal to just apply that, without introducing a Phorge-only getEnvConfigStr() method.