[New Rule] Function/method call in for loop declaration MUST be avoided

[lenaorobei]

Rule

Function/method call in for loop declaration MUST be avoided.

Reason

PHP will call the method/function on each iteration of the for loop.

Bad Example:

for ($i = 0; $i < count($array); $i++){
    //code goes here
}

Good example:

$count = count($array);
for ($i = 0; $i < $count; $i++){
    //code goes here
}

Implementation

• Subscribe to the T_FOR token.
• Check if condition contains function call (T_STRING with followed by open parentheses).

[ravmenon]

Hopefully some future versions of PHP compiler will optimize away such constructs to make this sniff unnecessary (most modern compilers handle this well), but for now, if we have this sniff, it should only be a warning.

[alexkiselov]

This rule requires better description.

The title confuses: I thought inside the loop itself, not in "for" statement definition.

[lenaorobei]

@akiseliov thanks for the feedback. I updated the title. Hope it makes more sense now.

[Zifius]

Hi @lenaorobei, something like this was done in the ECG Sniff: https://github.com/magento-ecg/coding-standard/blob/master/Ecg/Sniffs/Performance/LoopSniff.php

Maybe it makes sense to port it. What's your vision generally on those sniffs?

[lenaorobei]

Hi @Zifius. We use this rule for Magento Marketplace verification and I noticed a lot of false-positive findings. This is the reason why I didn't include this sniff and created a proposal. Performance/LoopSniff detects specific function/method calls inside the loop and from my point of view the detection of function call in the for loop declaration (for ($i = 0; $i < count($array); $i++)) gives more accurate results.

[orlangur]

@lenaorobei this rule is totally useless actually and was rejected endless amount of times, for instance: magento/magento2#17501 (comment)

[lenaorobei]

@orlangur I'm really interested in understanding your firm position regarding this rule.

Few points from my side about this:

1. This rule is not only for count, but for any function/method call in the for loop.
2. It's well-known "PHP bad practice" item. I agree that it is not about performance. It's more about code style. If we want to write stable and reliable code, we need to follow such best practices. If we can eliminate potential performance issues, we need to do this. It's like we encourage the use of strict comparison === to eliminate potential security issues because of type casting.
3. Once introduced the rule will be applicable to changed files and in future we will be able to fix legacy issues as well.

Looking forward to constructive dialogue.

[orlangur]

1. not only for count, but for any function/method call in the for loop

Calling function in loop signature is perfectly valid, just like calling within if condition, for example. For function in general such rule is not applicable as there is no guarantee it is idempotent.

2. It's well-known "PHP bad practice" item

Proof, please. All previous contributions I met were just about performance impact and I provided an evidence such concerns are groundless.

2. It's like we encourage the use of strict comparison === to eliminate potential security issues because of type casting.

Security aspect is only applicable to compare some hashes where string may be accidentally interpreted as number with == being in use.

3. Once introduced the rule will be applicable to changed files and in future we will be able to fix legacy issues as well.

Don't see how it is related to this issue, generally a real standard is something our code is 100% compliant with (like it was after magento/magento2#8685) and we can have additional checks for new files like it works for declare(strict_types=1). Basically, there is no need to distract contributors with "legacy" coding style issues, we must simply eliminate them.

So, to summarize this particular rule, there is no need to put some function call out of loop signature until profiler shows it is a bottleneck.

[lenaorobei]

The rule is already included from OOB Generic Coding Standard:

<rule ref="Generic.CodeAnalysis.ForLoopWithTestFunctionCall">
    <severity>7</severity>
    <type>warning</type>
</rule>

[orlangur]

@lenaorobei so what is an actual basis for this list #14? Why it wasn't discussed and cleaned up from useless rules like this one?