Skip to content

Report only accumulating array_merge in loops, cover while and do-while - #503

Open
lbajsarowicz wants to merge 1 commit into
magento:developfrom
lbajsarowicz:fix/foreach-array-merge-accumulator
Open

Report only accumulating array_merge in loops, cover while and do-while#503
lbajsarowicz wants to merge 1 commit into
magento:developfrom
lbajsarowicz:fix/foreach-array-merge-accumulator

Conversation

@lbajsarowicz

Copy link
Copy Markdown

Description

Magento2.Performance.ForeachArrayMerge warns on every array_merge token that appears
inside a for or foreach body. Two consequences:

It over-reports. Only the accumulating shape — the result merged back into one of the
call's own arguments — makes the loop quadratic. A call like

foreach ($sources as $source) {
    $row = array_merge($defaults, $source);   // constant work per iteration
}

is flagged today with nothing to fix, and so is a call whose target is rebuilt from scratch
earlier in the same iteration:

while (count($products) > 0) {
    $ids = array_column($products, 'entity_id');
    $ids = array_merge($ids, $related);       // does not grow across iterations
}

magento/magento2 2.4-develop currently carries 77 // phpcs:ignore Magento2.Performance.ForeachArrayMerge
annotations. Suppressing a warning line by line is the only available answer when the sniff
cannot tell the two shapes apart, and every suppression also hides the real finding if the
code later changes.

It under-reports. register() only returns T_FOREACH and T_FOR, so while and
do/while bodies are never inspected. Magento\Framework\View\Design\Fallback\Rule\Theme::getPatternDirs()
accumulates into $result inside a while loop and has never been reported.

What changed

A call is reported when it accumulates:

  • the assignment target appears among the arguments of the same call — covering $var,
    $this->property, self::$property and array offsets such as $data['rows']; or
  • the result is passed to a setter fed by a getter on the same object
    ($builder->setColumns(array_merge($builder->getColumns(), $columns)), the case already
    covered by the fixture from Better solution for fixing array_merge in loop #195);

and it is not obviously reset per iteration:

  • no unconditional reassignment of the target earlier in the same iteration at the same
    nesting level — a preceding $target = array_merge($target, ...) still counts as
    accumulation, so consecutive merges into one accumulator are both reported;
  • the target is not the value or key variable of the enclosing foreach.

register() now returns T_FOREACH, T_FOR, T_WHILE and T_DO, and every call is
reported by the innermost loop that contains it. The while of a do/while has no scope
of its own, so the T_DO body is what gets inspected — no double reporting. Inline
(brace-less) loop bodies keep being skipped, as before. A method or function named
array_merge is no longer mistaken for the global function.

The warning code and message are unchanged, so existing suppressions and baselines keep
working.

Effect on magento/magento2 2.4-develop

Measured with this branch against magento/magento2@b977e369455
(app/code/Magento, lib/internal/Magento, setup/src):

before after
warnings reported (suppressions honoured) 46 36
detections with --ignore-annotations 126 74

53 of the 126 raw detections were calls that cannot grow anything; one new finding appears
(Framework/View/Design/Fallback/Rule/Theme.php:78, the while accumulator above). No
accumulating call that the sniff reported before is missed.

Manual testing scenarios

  1. vendor/bin/phpunit --filter ForeachArrayMerge — the fixture grew from 3 to 11 expected
    warnings and now also pins the shapes that must stay silent (constant-work merge, target
    reset per iteration, foreach value/key variable, setter fed by a different object,
    method named array_merge).
  2. vendor/bin/phpcs --standard=Magento2 --sniffs=Magento2.Performance.ForeachArrayMerge <path to magento2>
    before and after, to reproduce the table above.
  3. vendor/bin/phpcs --standard=Magento2 Magento2/Sniffs and
    --standard=Magento2Framework Magento2/Sniffs — clean.

Full vendor/bin/phpunit also fails 9 Magento2\Tests\Eslint\* tests locally without
npm install; those fail identically on develop and are unrelated to this change.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

ForeachArrayMergeSniff warned about every array_merge token inside a for or
foreach body. Only the accumulating shape - the result merged back into one of
the call's own arguments - grows the array on every iteration, so calls like
`$row = array_merge($defaults, $data);` were reported without anything to fix,
and calls that reset the target earlier in the same iteration were too. At the
same time while and do-while bodies were never looked at.

The sniff now reports a call when:
- the assignment target is one of the arguments of the same call, or the result
  is passed to a setter fed by a getter on the same object, and
- the target is not unconditionally overwritten earlier in the iteration, and
  is not the value or key variable of the enclosing foreach.

Loop tokens registered are now T_FOREACH, T_FOR, T_WHILE and T_DO, and each
call is reported by the innermost loop that contains it.
@lbajsarowicz

lbajsarowicz commented Aug 5, 2026

Copy link
Copy Markdown
Author

Three examples of the shape this PR is meant to keep catching, each with before/after numbers, opened against 2.4-develop:

All three carried // phpcs:ignore Magento2.Performance.ForeachArrayMerge annotations, which this change makes unnecessary. They are the reason for the narrowing proposed here: 53 of the 126 raw detections in 2.4-develop (42 %) cannot grow anything, so the annotation becomes the default response and genuine findings like these sit behind it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant