Skip to content

fix: keep & on foreach targets that are not plain variables - #2549

Open
wleeaf wants to merge 1 commit into
prettier:mainfrom
wleeaf:foreach-byref-target
Open

wleeaf wants to merge 1 commit into
prettier:mainfrom
wleeaf:foreach-byref-target

Conversation

@wleeaf

@wleeaf wleeaf commented Oct 25, 2026

Copy link
Copy Markdown

Bug: foreach can assign by reference to a property, an array element or a static property, not only to a variable:

foreach ($test as &$object->property) {}
foreach ($test as $key => &$array[$key]) {}

The parser marks the value with byref, but only the variable printer prints the &, so these were formatted as foreach ($test as $object->property) and foreach ($test as $key => $array[$key]). The loop then assigns by value instead of binding a reference, which silently changes what it does.

Fix: in the foreach printer (src/printer.mjs), a byref value that is not a plain variable gets the & printed in front of it. Variables still print their own &, so foreach ($a as &$v) is unchanged.

Tests: added both forms above to tests/foreach/foreach.php and updated the snapshot. Without the fix, the snapshot test fails because the & is gone. With it, yarn test:node passes (599 tests), AST_COMPARE=1 yarn test:node passes and yarn lint is clean.

I found this by formatting the --FILE-- sections of php-src's Zend/tests and comparing nikic/PHP-Parser ASTs of the input and output. Zend/tests/foreach/foreach_by_ref_to_property.phpt and Zend/tests/enum/no-write-properties-through-foreach-reference.phpt changed meaning before this change and round-trip after it.

foreach can assign by reference to a property, an array element or a
static property, not just to a variable. The parser marks such a value
with byref, but only the variable printer prints the &, so

    foreach ($test as $key => &$array[$key]) {}

was formatted as `foreach ($test as $key => $array[$key]) {}`, which
assigns by value and silently changes what the loop does.

The foreach printer now adds the & itself for a byref value that is not
a variable.
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