Skip to content

Throw a SyntaxError instead of a PHP fatal error when a macro argument is defined twice#4853

Merged
fabpot merged 1 commit into
twigphp:3.xfrom
fabpot:3.x
Jul 4, 2026
Merged

Throw a SyntaxError instead of a PHP fatal error when a macro argument is defined twice#4853
fabpot merged 1 commit into
twigphp:3.xfrom
fabpot:3.x

Conversation

@fabpot

@fabpot fabpot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@upsun-dispatch

upsun-dispatch Bot commented Jul 4, 2026

Copy link
Copy Markdown

📋 PR Summary

This PR changes Twig's macro compilation so that defining a macro argument twice raises a proper SyntaxError instead of a PHP fatal error. It updates MacroNode to detect duplicate argument names, adjusts TempNameExpression, and adds test fixtures covering duplicate and duplicate-reserved argument cases.

Changes
Layer / File(s) Summary
Macro argument validation
src/Node/MacroNode.php Adds detection of duplicate macro argument names and throws a SyntaxError instead of allowing a PHP fatal error.
src/Node/Expression/TempNameExpression.php Minor adjustment supporting the macro argument handling change.
Tests
tests/Fixtures/macros/duplicate_argument.test New fixture verifying a SyntaxError is thrown when a macro argument is defined twice.
tests/Fixtures/macros/duplicate_reserved_argument.test New fixture verifying a SyntaxError is thrown for duplicate reserved argument names.
Changelog
CHANGELOG Documents the new SyntaxError behavior for duplicate macro arguments.

@upsun-dispatch upsun-dispatch Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📋 Upsun Dispatch Review: full · 5 files reviewed · no issues found

Review details

Commit: Commit 0d283d3
Model: claude-opus-4-8
Panel: correctness · robustness · design

@fabpot fabpot merged commit d1abdf4 into twigphp:3.x Jul 4, 2026
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant