I would like to see a full stack trace for this thought, since the function is annotated to only receive a string, and null is not a string, so we should not pass null to it if possible.

In D25164#5066, @valerio.bozzolan wrote:

I would like to see a full stack trace for this thought, since the function is annotated to only receive a string, and null is not a string, so we should not pass null to it if possible.

Makes sense! (This ticket was filed before I managed to get good traces.) Stacktrace added to T15263.

Since my inline edit is probably too much confusing,

In short I just suggest this short-circuit that replicates the behavior before PHP 8.1:

function phutil_escape_uri($string) {
+ if($string === null) {
+   return '';
+ }
  return str_replace('%2F', '/', rawurlencode($string));
}

If you want easy extra points, maybe this can be even more nice, using phlog():

function phutil_escape_uri($string) {
+ if($string === null) {
+   phlog("Received phutil_escape_uri(NULL). Please share this stack trace in https://we.phorge.it/T15263");
+   return '';
+ }
  return str_replace('%2F', '/', rawurlencode($string));
}

In D25164#5079, @valerio.bozzolan wrote:

Since my inline edit is probably too much confusing,

In short I just suggest this short-circuit that replicates the behavior before PHP 8.1:

function phutil_escape_uri($string) {
+ if($string === null) {
+   return '';
+ }
  return str_replace('%2F', '/', rawurlencode($string));
}

If you want easy extra points, maybe this can be even more nice, using phlog():

function phutil_escape_uri($string) {
+ if($string === null) {
+   phlog("Received phutil_escape_uri(NULL). Please share this stack trace in https://we.phorge.it/T15263");
+   return '';
+ }
  return str_replace('%2F', '/', rawurlencode($string));
}

Also in this patch @aklapper feel free to update this, to maintain authorship

Or, feel free to say "yeah whatever, just update this with the proposed fix". In that case I will probably also "Commander" the revision in order to be the author since my "tip" here is too much invasive compared with the original (but in this case I will not be able to approve myself and see this landed quickly)

@valerio.bozzolan: Ah, thanks. If there is an assumption that string passed to phutil_escape_uri($string) shall never ever be null and in that case there is always an underlying bug to fix (that wasn't clear to me), then I'd say adding a phlog() makes sense if it automagically includes a stacktrace and has clear instructions what a random Phorge admin seeing that log entry is supposed to do (like "Unexpected null string passed to phutil_escape_uri. Please report a bug at https://we.phorge.it/maniphest/ and mention T15263 in your task.") Okay.