Problem/Motivation

The performance pattern static_service_in_loop (modules/audit_performance/src/Plugin/AuditAnalyzer/PerformanceAnalyzer.php) reports any \Drupal::service(, ::entityTypeManager(, ::currentUser(, ::database( or ::cache( call inside a loop, recommending to "Move \Drupal:: calls outside loop or use dependency injection". The argument is never inspected. Code like

foreach ($component_list as $type => $name_list) {
$manager = \Drupal::service('extension.list.' . $type);

resolves a DIFFERENT service on each iteration, so the call cannot be moved out of the loop as the recommendation asks. The same applies to a service ID taken from the loop value and to \Drupal::cache($bin) with a bin read inside the loop.

Steps to reproduce

1. Add the loop above to a .module file.
2. Run drush audit:run performance --format=json.
3. STATIC_SERVICE_IN_LOOP is reported.

Proposed resolution

Add a skip_loop_dependent_argument flag to the pattern, handled in isContextualPerfIssue() by a new hasLoopDependentServiceArgument() helper: skip the call when the argument of \Drupal::service() or \Drupal::cache() uses a variable set by the innermost loop, i.e. a variable of its header (foreach ... as $key => $value, for/while assignments) or one assigned in its body before the call.

Calls that can still be moved out of the loop keep being reported:
- literal service IDs;
- variables defined before the loop;
- the argument-less wrappers (entityTypeManager(), currentUser(), database());
- an ID that depends on an OUTER loop variable, inside an inner loop (the call can be moved out of the inner loop).

Earlier reports proposed skipping every non-literal argument; that was rejected because a variable defined before the loop is as movable as a literal.

Remaining tasks

- Implement the flag and the helper.
- Unit test with a data provider (none covered STATIC_SERVICE_IN_LOOP before): ID built from the loop key, ID from the loop value, cache bin assigned in the loop => not reported; literal ID, ID variable defined before the loop, currentUser(), outer loop variable inside an inner loop => reported.

User interface changes

STATIC_SERVICE_IN_LOOP is no longer reported for service IDs that change on each iteration.

API changes

None.

Data model changes

None.

Comments

trebormc created an issue. See original summary.

  • trebormc committed 4cdea4c5 on 1.x
    Issue #3630473 by trebormc: Skip loop-dependent service IDs in the...
trebormc’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.