My suggestion is to simply add this to the ForbiddenFunctions sniff. I suggest to not invest time on an auto-fix. Code that calls isset( …, … ) with multiple arguments is so rare, it's probably more effective to just manually fix these few places than to write, review, and maintain an auto-fix.

Please note that an auto-fix must take the operator precedence into account – as always. Blindly replacing isset( …, … ) with isset( … ) && isset( … ) can potentially break code.

Please note that an auto-fix must take the operator precedence into account – as always. Blindly replacing isset( …, … ) with isset( … ) && isset( … ) can potentially break code.

I agree!

Also, unset( ..., ... ) is valid too but in our code bases, we rather use multiple unset() statements. Maybe this is something to also address here as well?

I would support this

When not seeing this often it is hard to find each argument and may read it as one isset, the forbidden function sniff already supports function with an count to be forbidden, but in this case the condition must be inverted (because it should be exactly one, not zero or two)

+1, I too think calling isset() for each expression separately is more readable than a single combined call.

The concept of checking whether a variable is set is quite common and when reading, debugging, or reviewing code this can become a mental concept one can quickly recognise visually and understand.

When merging two or more unrelated checks into a single expression, it would require mental unpacking to understand, and would require a larger range over which to remember what is being done (given the function and expression are no longer attach directly).

This is kind of unpacking is something we do very commonly for standalone statements, and for boolean operators within an if statement (e.g. multiple checks combined with &&), but it's rather unusual to see for inline function calls.

If there is a truly large number of things to check, one might go for a multi-line statement with its result stored in a separate variable (possibly using a spread operator), e.g.:

$yallExist = isset(
 $x['a'],
 $x['b'],
 $x['c']
);
if ( $yallExist && $other–>isStuff ) { .. }

That seems like quite unlikely to me. If we do need this on rare occasions, we can disable the rule ad-hoc as needed. If that becomes common, though, we should consider to having it be part of our codesniffer by default given that any kind of friction is imho likely to quickly tip the balance of adde value vs cost.

But, it sounds like people agree it's worth trying out :)

Multi-parameter unset() seems harmless to me, and probably more readable than separate unset commands. Unlike isset() (where it is not obvious whether isset( $x, $y ) unpacks to isset( $x ) && isset( $y ) or isset( $x ) || isset( $y )) there is no way to misread it.

@Tgr, thanks for your input. Looking at the documentation here: https://www.php.net/manual/en/function.isset.php, it's kinda clear that using multiple arguments translates to an AND for the underlying implementation of this function because of If multiple parameters are supplied then isset() will return TRUE only if all of the parameters are considered set.

So, isset( $x, $y ) translates to isset( $x ) && isset( $y ).

An aside

In line with this kind of convention, I've seen in our code bases lines of code like;

$x = $y = $z = 0;

instead of

$x = 0;
$y = 0;
$z = 0;

I think this is related in the sense that, though the above is a function call with all arguments passed to the caller, it's a one-liner as well. I'm thinking whether it's related to the discussion here? But in any case, this was just an aside.

Having to look at the documentation to interpret something meets the definition of "not obvious".

Change 551639 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/tools/codesniffer@master] Avoid use of isset( ..., ..., [...] ) function construct / syntax

https://gerrit.wikimedia.org/r/551639

Change 551639 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Avoid use of isset( ..., ..., [...] ) function construct / syntax

https://gerrit.wikimedia.org/r/551639

The last question is, if this should be done also for unset()

I would say yes, because it makes the code easier when doing one thing in one line and not combine multi unsets in one line.