Page MenuHomePhorge

Fix strlen(null) errors for projects with integer fields under PHP 8.1

Authored by Sten on Sep 20 2023, 20:04.
Referenced Files
Unknown Object (File)
Sat, Nov 25, 21:22
Unknown Object (File)
Mon, Nov 20, 21:42
Unknown Object (File)
Mon, Nov 20, 21:42
Unknown Object (File)
Fri, Nov 17, 14:59
Unknown Object (File)
Sat, Nov 11, 15:07
Unknown Object (File)
Wed, Nov 8, 04:11
Unknown Object (File)
Tue, Nov 7, 10:34
Unknown Object (File)
Mon, Nov 6, 19:19



If you have a custom integer field defined under projects.custom-field-definitions, then
when creating or editing a project under PHP 8.1 it will fail with strlen(null) errors.

As the integer fields have int values, we can't replace the strlen() with phutil_nonempty_string(), but replacing it with a simple null check does the job.

Test Plan

Test creating a project when using a custom integer field:

  "mycompany:intfield": {
    "name": "Int Field",
    "type": "int",
    "description": "Some int field "

Fixes T15641

Diff Detail

rP Phorge
project (branched from master)
Lint Passed
Tests Passed
Build Status
Buildable 848
Build 848: arc lint + arc unit

Event Timeline

Sten requested review of this revision.Sep 20 2023, 20:04


I'm generally scared by strlen() things since it means it was probably relying on implicit cast-to-string.



But it will become unreadable with many if(not-nonempty)

Looking at the code, your solution seems just perfect. But I have not tested all combinations of null/0/1/"0"/"1"/""/"lizard" on $old and $new before and after.


phutil_nonempty_scalar() just here?

or stringlike()?

These with explicit casting (as before from strlen) may be necessary?

I need coffee


When testing, I first tried phutil_nonempty_string() and it bombed out with exception 'got an int, but expected a null or str. Hardly surprising given that this class is specifically for handling an int value.

So phutil_nonempty_scalar() would work here, but as we know it will only be called for an int, a null check is sufficient.

Also, as this is only for a custom field, it's fairly safe - it won't touch default installs.

Is switching to using phutil_nonempty_scalar a blocker?
I don't think it should be because otherwise, we could have just done a bulk change across the whole codebase converting

if (strlen($var))


if (phutil_nonempty_scalar($var))

and saved ourselves much work. The whole point of not doing the bulk change (I think) was to implement tighter controls where possible, such as here.

However, I will change it to phutil_nonempty_scalar if that's what's required.

(Indeed I agree we cannot bulk edit things - PHP 8.1 is a nightmare - see T15190)

Essentially here we are handling a Transaction, that can receive wild values from the user, that should be rendered as integer, and persisted as NULL or integer.

In general, the implicit logic is Phabricator was:

Receive wild thing -> cast to string -> int

Practical examples of what should be considered wild but usually accepted:

  • null
  • int
  • strings (and some of them can be casted to int)
  • maybe objects (and some of them can be casted to strings - see above - and so, some of them can be casted to int)

Before PHP 8.1, all the above were valid inside if(strlen($v)), and that check was answering the question "Is this value non-empty when casted as string?" and, that's why I was thinking about if(phutil_nonempty_scalar()) that is probably the 1:1 readable conversion in this situation - but trust me - I don't know.

Hard block:

In my opinion all lines cause a regression since they do not consider anymore these cases:

  • empty string ""
  • objects that can have a __toString() resulting in an empty string
  • maybe other non-null things that, casted to string, result in an empty string

So, for an hard approval from my side, I can suggest one of these:

  1. the general ugly fix that always work :D
-if (strlen($v)) {
+if ($v !== null && strlen($v)) {
  1. or, adopt $is_nonempty = phutil_string_cast($value) !== ''; (already in use in line 27)
  2. or, adopt if(phutil_nonempty_scalar($value)) (already in use in line 36)

I would honestly roll a dice to discover what would be better


(See this interesting line)


(See this interesting line)

This revision now requires changes to proceed.Mon, Nov 20, 16:05

I can no longer replicate the fault, so am going to scrap this one.

Probably this happens while creating a new project and doing that thing again 🤔