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
Comment #3
trebormc