Problem/Motivation
PerformanceAnalyzer::isInsideLoop() finds the last foreach / for( / while( keyword in the 30 lines before the target line and counts braces from that keyword up to the end of the target line. When the keyword is on the target line itself, the opening '{' of that same loop is counted, the depth becomes 1 and the method returns TRUE. Reproduced on current 1.x with drush audit:run performance:
foreach ($entity->get('field_ref')->referencedEntities() as $term) { => N1_REFERENCED_ENTITIES
foreach ($storage->loadMultiple() as $pattern) { => N1_LOAD_MULTIPLE
The iterable expression runs exactly once, so it is not an N+1 pattern. The existing isInsideLoop tests do not cover a match on the keyword line.
Note: LOAD_MULTIPLE_ALL is also reported on the second line, but that rule does not depend on the loop context (it flags any loadMultiple() without arguments), so it is not affected by this bug.
Proposed resolution
Only consider loops that start before the target line. Code on the line that starts a loop belongs to that loop's iterable expression and runs once, so a loop starting on the target line is ignored and the check falls back to any enclosing loop. This keeps a real N+1 when the iterable of a nested loop runs once per iteration of an outer loop:
foreach ($items as $item) {
foreach ($item->referencedEntities() as $ref) { => still N1_REFERENCED_ENTITIES
Simply returning FALSE when the keyword is on the target line would have hidden that case.
A loop body written on the same line as its keyword (foreach (...) { $x->load(); }) is no longer reported; that layout does not follow the Drupal coding standards.
Tests: data provider cases for the iterable expression (FALSE), a call in the loop body (TRUE), the iterable of a nested loop (TRUE) and a call after the loop (FALSE). The first case fails without the fix.
Remaining tasks
- Review the merge request.
Comments
Comment #3
trebormc