Shorthand operators _should_ be nested (php:S3358)

I’m getting php:S3358 “Ternary operators should not be nested” on a line that chains (nests really isn’t even the right word) several shorthand ternary and null coalescing operators. In my specific case, I’m trying to extract an api key from various sources and use the first one that is set, according to some priority. My code looks like this:

$api_key = $arg_key ?: $request_key ?: $_COOKIE['api_key'] ?? '';

This is, in my view, absolutely easy to understand, assuming the reader knows what the operators mean (which is a fair assumption, otherwise the rule should be “Do not use operators someone might not know”):

"Set the variable named api_key to the first valid value you find in the following list:

  • $arg_key (valid = not falsey)
  • $request_key (valid = not falsey)
  • $_COOKIES[‘api_key’] (valid = not null)
  • default: empty string"

The compliant code according to the rule would look like this:

$api_key = $_COOKIES['api_key'] ?? '';
$api_key = $request_key ?: $api_key;
$api_key = $arg_key ?: $api_key;

which reverses the priority in order to avoid a potential Notice: undefined index, making the priority less clear. Alternatively, one could use an if-chain like so:

$api_key = '';
if ($arg_key) {
    $api_key = $arg_key;
} elseif ($request_key) {
    $api_key = $request_key;
} elseif (isset($_COOKIES['api_key'])) {
    $api_key = $_COOKIES['api_key'];
}

Those can also be combined in an inconsistent mess of styles. Either way, the compliant code is significantly more verbose and complex, while arguably actually more difficult to understand. Shorthand operator chaining is as clear as it gets when a variable should be set to the first valid value out of a selection of more than 2. This gets more expressed the larger the list of potential values becomes.

I would thus like to request the following feature/change:
Restrict rule php:S3358 to real ternary operators (a ? b : c), excluding the shorthand ternary operator (a ?: b) and the null coalescing operator (a ?? b).

I really don’t want to disable the rule because it’s a very good rule and makes a lot of sense as long as it flags actual (non-shorthand) ternary operators being nested inside one another. But for chaining shorthand operators in sequence, it’s counter-productive since that should actually be encouraged.

Hi @scenia,

Welcome to SonarSource Community!

Thanks for your suggestion, we will discuss it in the team, and come back to you with an answer.

Roberto

1 Like

Hi Roberto,

thanks for your reply, looking forward to your discussion’s result!

After some more testing and playing around, I dicovered that the null coalescing operator (a ?? b) is already excluded.
I was able to get rid of the flag by restructuring my code to use nulls instead of falsey values and then replace the ?: operators with ??, but that feels wrong because those values shouldn’t really be nullable.

So essentially, the suggestion becomes “Treat the shorthand ternary operator the same way the null coalescing operator is treated, rather than the full ternary operator.”

Hello @scenia,

After discussing your suggestion in the team, we think that you raised a fair point, and indeed raising this issue for shorthand ternary operators can be noisy.
Thus we will exclude shorthand operators from this rule.

I have created the ticket SONARPHP-1115 so you can be up to date.

Thank you again for being an active member of our community.

Best,
Roberto

Awesome! Looking forward to it :slight_smile: