Gentle reminder that this has been waiting review for a while now...

tl;dr: I like phutil_nonempty_string($map['query']) better if we know $map['query'] is defined, and phutil_nonempty_string(idx($map, 'query')) otherwise.

Yeah, we can use isset() and strlen() in place of phutil_nonempty_string(), and I'm happy to do it.

However, if this is canon, then under what circumstances should phutil_nonempty_string be used?

I think (because we didn't actually formalized any of this) that phutil_nonempty_string($x) is generally for cases where $x is known to be "string or null", vs strlen($x) which is for $x that is "string" (not null).

Here, since $map is an array, we get another possible state for $map['query'] of "undefined", which isn't the same, but might be converted to null (or even a string) by something. Well, non-array $x can be "undefined" as well, but we have lints to prevent that.

if $map['query'] is undefined, we would throw an exception here, so the isset check here is equivalent to $map['query'] == null.
I'm pretty sure that canonically, we use isset only in the meaning of array_key_exists.

<?php

require_once dirname(__FILE__).'/__init_script__.php';

$y = array("a" => 3, 'b'=>null);
echo "\ny[a]: ";
echo $y['a'];
echo "\nisset(y[b]) : ";
echo isset($y['b'])  ?'set': 'unset';
echo "\nisset(y[c]) : ";
echo isset($y['c'])  ?'set': 'unset';
echo "\ny[c]  : ";
echo $y['c'];  // <-- Exception `Undefined array key "c"`
echo "\n";

Gotcha. So

• isset() is safer when pulling things out of arrays.
• phutil_non_empty_string() provides for stricter parameter checking as it demands the array value is set, even if set to null.

Given that both work in this instance, we have a choice of code robustness or strictness. I'm inclined to go for robustness and use isset as suggested by @speck, but but don't mind either way - my priority is getting it to work under PHP 8.1.

Hi all, can this be approved if no one has any objections? Dashboards simply can't be modified under PHP 8.1 without this. Thanks.