Thanks

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

Maybe

phutil_nonempty_scalar()

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.

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))

to

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)) {

2. or, adopt $is_nonempty = phutil_string_cast($value) !== ''; (already in use in line 27)
3. 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

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