Page MenuHomePhabricator

Browsing directories with submodules fails with bad call to "setExternalURI()"
Closed, ResolvedPublic

Description

In D21511, I renamed the iterator variable in this loop from $path to $submodule_path, but incorrectly did not modify the $path->setExternalURI() call:

foreach ($submodules as $submodule_path) {     // <<< Renamed.
  $full_path = $submodule_path->getFullPath();
  $key = 'submodule.'.$full_path.'.url';
  if (isset($dict[$key])) {
    $path->setExternalURI($dict[$key]);        // <<< Now incorrect.
  }
}

This scope has a different variable named $path (which the rename was attempting to avoid clobbering), so the static analyzer reasonably couldn't detect this issue.

However, the existence of the original code might point at a bug in the "Variable Reused as Iterator" lint check: I would expect it to have prevented the original code in the first place.

Event Timeline

epriestley triaged this task as Normal priority.Jan 26 2021, 4:37 PM
epriestley created this task.

However, the existence of the original code might point at a bug in the "Variable Reused as Iterator" lint check: I would expect it to have prevented the original code in the first place.

Currently, the linter allows a variable to be reused as an iterator so long as it is not referenced after the loop where it is reused. This might be worth making more strict, but code in this form isn't strictly incorrect and the linter is working as intended.