Page MenuHomePhorge

phutil_nonempty_scalar(): don't throw when receiving a boolean scalar
ClosedPublic

Authored by valerio.bozzolan on Apr 7 2023, 16:26.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 17, 06:02
Unknown Object (File)
Thu, Apr 11, 05:53
Unknown Object (File)
Thu, Apr 11, 04:26
Unknown Object (File)
Sun, Apr 7, 09:09
Unknown Object (File)
Mon, Apr 1, 01:12
Unknown Object (File)
Mon, Apr 1, 01:11
Unknown Object (File)
Mon, Apr 1, 01:11
Unknown Object (File)
Mon, Apr 1, 01:11

Details

Summary

Note that booleans are scalars. Full stop.

is_scalar($v)Result
"foo"true
""true
nulltrue
0true
0.5true
truetrue
falsetrue
new stdclass()false
array()false

Note that phutil_nonempty_scalar() was designed just to tell
whenever a scalar is "empty" or not. So it must not explode
when receiving a valid scalar.

So the question is not whenever a boolean is a scalar or not,
but whenever is empty or not. But also this is a clear fact:

$vis_scalar($v)!is_empty($v)if(strlen($v))
truetruetruetrue
falsetruefalsefalse

In short, now the function does not explode anymore with bool
values. Instead, it says whenever are empty or not.

In bold the exact changes:

Valuephutil_nonempty_scalar($v)
"foo"true
""false
nullfalse
0true
0.5true
obj with tostringtrue
obj withno tostr.Exception
array()Exception
trueException true
falseException false

Closes T15239

Test Plan
  • check if it makes sense to you
  • check the few usages

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

update 1 unit test, add 1 unit test

src/utils/__tests__/PhutilUtilsTestCase.php
1018

This is a nightmare to read but it just reflects the above change to require not an exception. So:

Subjecttest nonempty stringtest nonempty string liketest nonempty scalarHuman test name
falsenullnullnull falsebool "false bool"
truenullnulltrue"true bool"

Where it's NULL it means: return an exception.

At the moment this is safe since we have just two usages from Phorge:

$ grep -R 'phutil_nonempty_scalar' --include="*.php" .
./src/applications/repository/storage/PhabricatorRepository.php:    if (phutil_nonempty_scalar($commit)) {
./src/applications/daemon/event/PhabricatorDaemonEventListener.php:    if (phutil_nonempty_scalar($context)) {

The first thing relies on the fact that the input must be cast-able to a string to be used in str_replace(). And that is.

The second thing, the same.

add inline comment to explain old behavior

valerio.bozzolan retitled this revision from phutil_nonempty_scalar(): don't throw when receiving booleans to phutil_nonempty_scalar(): don't throw when receiving a boolean scalar.Jun 27 2023, 12:36
This revision is now accepted and ready to land.Jul 17 2023, 06:06