Page MenuHomePhorge

Fix PHP 8.1 "strpos(null)" exception from PhutilCommandString which blocks arc patch
ClosedPublic

Authored by valerio.bozzolan on May 8 2023, 16:53.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 21, 23:20
Unknown Object (File)
Sat, Apr 20, 06:17
Unknown Object (File)
Thu, Apr 11, 13:00
Unknown Object (File)
Thu, Apr 11, 07:52
Unknown Object (File)
Thu, Apr 11, 04:36
Unknown Object (File)
Sun, Apr 7, 05:50
Unknown Object (File)
Mon, Apr 1, 01:35
Unknown Object (File)
Sun, Mar 31, 12:34

Details

Summary

For some reason it may happen that a specific command line argument receives a null argument
from PHP, instead of just an empty string.

Nowadays, this null value probably breaks almost whatever GNU/Linux or FreeBSD or Microsoft Windows
etc. implementations since everyone expect to receive a string.

This used to work in the past since functions like strpos() or strlen() accepted null, but not
anymore. This generate a deprecation warning since PHP 8.1, that is elevated as exception from
Phabricator/Phorge and breaking features.

Without getting into implementation logics (which doesn't make sense to fix all of them) the
calling function should just be kind. So we normalize nonsense null values to an empty string.

Note: this was the expected behavior prior to PHP 8.1.

Now we do that normalization explicitly, in this early point.

After this fix, also T15368 should probably be fixed.

Closes T15367

Test Plan
  • run "arc patch <something valid>"
  • to you it must continue to work
  • (to @ton it starts working right now)

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.May 9 2023, 06:27

I don't think this is an appropriate change, instead passing null into PhutilCommandString should be fixed. From the trace on T15367 it looks like a more appropriate fix might be updating ArcanistGitAPI::hasLocalCommit() to return false if $commit is null, or possibly updating ArcanistDifferentialDependencyGraph::loadEdges to check for null. I'm not sure under what circumstances it would end up with a null value though. In some basic testing the query it's doing does not return null for me.

In D25205#7538, @speck wrote:

I don't think this is an appropriate change, instead passing null into PhutilCommandString should be fixed. From the trace on T15367 it looks like a more appropriate fix might be updating ArcanistGitAPI::hasLocalCommit() to return false if $commit is null, or possibly updating ArcanistDifferentialDependencyGraph::loadEdges to check for null. I'm not sure under what circumstances it would end up with a null value though. In some basic testing the query it's doing does not return null for me.

Thanks. A follow-up patch from you is welcome.