Page MenuHomePhorge

fix PHP 8 "strlen(null)" when testing the ssh setup
Needs RevisionPublic

Authored by jeanguyomarch on Dec 2 2023, 09:36.

Details

Summary

As advised in "Diffusion User Guide: Repository Hosting", the following command may
be executed to test that ssh is correctly setup:

echo {} | ssh vcs-user@phorge.yourcompany.com conduit conduit.ping

This raised the deprecation error for strlen(null) with PHP 8.

Closes T15681

Test Plan

Run the aforementioned command

Diff Detail

Repository
rARC Arcanist
Branch
master
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningsrc/utils/utils.php:1955TXT3Line Too Long
Unit
Tests Passed
Build Status
Buildable 962
Build 962: arc lint + arc unit

Event Timeline

Fix warning raised by the linter.

Is there a stacktrace to work from here? I suspect nothing should really be passing null into phutil_encode_log() and there's likely another issue here. I prefer preventing passing null values into this rather than papering over the issue here in the depths of util functions.

src/utils/utils.php
1955–1958

Doing some testing I think the behavior of addcslashes() when it's passed a null is to return the empty string and not null. Looking at the callers of phutil_encode_log the return value is used in 2 different manners:

$value = '';
//...
$value .= phutil_encode_log(...);

The .= appears to do the same thing with null as it does empty string, so this case doesn't seem to matter.

The other scenario where the result is used:

pht('Blah blah %s blah', phutil_encode_log(...)

For which I'm not sure the behavior of pht() though I'd guess it'd be okay.

After reverting my patch, I have the following backtrace:

[2023-12-02 20:15:57] ERROR 8192: addcslashes(): Passing null to parameter #1 ($string) of type string is deprecated at [<MY_INSTALLATION_DIR>/arcanist/src/utils/utils.php:1955]
arcanist(head=5bc53cfe53d0afe813b19f28d6151273e7b86499, ref.master=d8e25bf27eef), phorge(head=jgu/php8, ref.master=cf8d5d60a594, ref.jgu/php8=358b3dc025fc)
  #0 addcslashes(NULL, string) called at [<arcanist>/src/utils/utils.php:1955]
  #1 phutil_encode_log(NULL) called at [<arcanist>/src/filesystem/PhutilDeferredLog.php:232]
  #2 PhutilDeferredLog::format() called at [<arcanist>/src/filesystem/PhutilDeferredLog.php:178]
  #3 PhutilDeferredLog::write() called at [<arcanist>/src/filesystem/PhutilDeferredLog.php:155]
  #4 PhutilDeferredLog::__destruct()

I don't know PHP, but I could not get any error by providing null to addcslashes() (i.e., when processing a php file by hand)... which I don't quite understand.
If that helps, I'm using PHP 8.2.7 bundled by debian 12.

Hmm that stack trade doesn’t seem to contain the origin of the issue, likely getting lost through error handling or maybe it’s an incomplete stack trace.

For testing your own php you will need to update the php.ini to set the mode it runs in. There’s a setting (I forget which) that has something like
+E_ALL ~E_DEPRECATIONS

Which tells it to not log deprecation warnings. If you remove the ~E_DEPRECATIONS then you will see logged warnings when you try out passing null in.

Some help is needed there, testing on PHP 8.1+ and getting the right stack trace

This revision now requires changes to proceed.Jan 16 2024, 16:17