Skip to content

Redesign macro calls and argument handling#4851

Open
fabpot wants to merge 1 commit into
twigphp:3.xfrom
fabpot:macro-ng
Open

Redesign macro calls and argument handling#4851
fabpot wants to merge 1 commit into
twigphp:3.xfrom
fabpot:macro-ng

Conversation

@fabpot

@fabpot fabpot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

This is my attempt to make macros "better". It uses modern PHP features that didn't exist when I designed macros a long time ago.

The first objective is to close the gap between their behavior and the behavior of Twig callables: functions, filters, and tests.

Here are some important changes:

  • Calling a macro without passing a value for an argument that has no default value is deprecated; it is currently silently passed as null.
  • Passing extra positional arguments or unknown named arguments to a macro without an explicit variadic argument is deprecated; these arguments are currently silently accepted through the implicit varargs variable.
  • Explicit variadic macro arguments are now supported with ...name.
  • Macros are compiled as closures stored in the macro registry, instead of public generated macro_* methods.

The refactor introduces TwigMacro and MacroArgument to represent template-defined macros with an explicit signature, similar to the existing Twig callable model.

Comment thread src/ExpressionParser/Infix/FunctionExpressionParser.php Outdated
Comment thread src/ExpressionParser/Infix/DotExpressionParser.php Outdated
Comment thread src/TokenParser/MacroTokenParser.php Outdated
Comment thread src/Template.php Outdated
Comment thread tests/Fixtures/macros/varargs.test
Comment thread tests/Fixtures/macros/simple.test
Comment thread src/TokenParser/FromTokenParser.php Outdated
// both "import" and "from" now route through Template::callMacro(), the two paths
// only differ in this parse-time symbol plumbing. In 4.0, unify "from" onto the
// "template" import symbol and remove the dedicated "function"-symbol branch in
// FunctionExpressionParser.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How can this actually happen ? The source being parsed looks like a function expression in the case of a usage based on from.

@upsun-dispatch

upsun-dispatch Bot commented Jul 3, 2026

Copy link
Copy Markdown

📋 PR Summary

This PR redesigns Twig's macro call and argument handling: macro invocation is unified through new Template::callMacro/resolveMacro/hasMacro entry points and a new TwigMacro/MacrosNode abstraction, replacing the previous macro_-prefixed compiled methods and getTemplateForMacro hook. It also adds variadic macro argument support, updates the expression/token parsers to route import and from ... import calls through MacroReferenceExpression, and includes extensive new tests, fixtures, and documentation.

Changes
Layer / File(s) Summary
Macro call & resolution redesign
src/Template.php Adds public callMacro/hasMacro plus resolveMacro/getDeclaredMacro/findDeclaredMacroName helpers that walk the parent chain to resolve macros; removes the getTemplateForMacro hook.
src/TwigMacro.php New class encapsulating a macro's runtime representation, argument binding, and deprecation handling.
src/Node/Expression/MacroReferenceExpression.php Slimmed down to reference macros via the new resolution scheme.
src/Node/MacroNode.php Reworked macro compilation, adds variadic/argument handling and deprecation annotations.
src/Node/MacrosNode.php New node aggregating a template's macros.
Expression parsing
src/ExpressionParser/Infix/ArgumentsTrait.php Minor adjustments to argument parsing.
src/ExpressionParser/Infix/DotExpressionParser.php Simplified dot-expression handling related to macro references.
src/ExpressionParser/Infix/FunctionExpressionParser.php Resolves imported macro symbols into MacroReferenceExpression.
src/Node/Expression/MethodCallExpression.php Small updates aligned with the new macro call path.
src/Node/Expression/TempNameExpression.php Minor addition supporting the new flow.
Token parsers
src/TokenParser/MacroTokenParser.php Extended to support variadic argument declarations and new validation.
src/TokenParser/FromTokenParser.php Registers from ... import aliases as imported function symbols routed through the unified macro resolution.
Core wiring
src/Environment.php Minor version/wiring adjustments.
src/Extension/CoreExtension.php Removed obsolete macro-related helper code.
src/Parser.php Small change supporting macro node aggregation.
Documentation
CHANGELOG Documents the macro redesign and deprecations.
doc/deprecated.rst Adds deprecation notes for the old macro mechanisms.
doc/tags/macro.rst Documents new variadic arguments and updated macro semantics.
Tests
tests/CallMacroTest.php New tests covering the macro call path.
tests/TwigMacroTest.php New tests for the TwigMacro class.
tests/Node/MacrosTest.php New tests for MacrosNode.
tests/Node/MacroTest.php Updated macro node compilation tests.
tests/Node/Expression/MacroReferenceTest.php Adjusted for the slimmed MacroReferenceExpression.
tests/Extension/SandboxStateChangeTest.php Minor updates to reflect new call path.
tests/Fixtures/macros Adds/updates fixtures for variadic macros, reserved names, non-identifier constants, and implicit optional arguments.
tests/Fixtures/tags/macro Updated tag fixtures for the new macro output.

@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 · 40 files reviewed · no issues found

Review details

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

@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: incremental · 42 files reviewed · no new issues

Review details

Commit: Commits dd55a6e..184ed57
Model: claude-opus-4-8
Panel: correctness · robustness · design

@fabpot

fabpot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

@stof I think I've taken all your comments into account.

@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: incremental · 42 files reviewed · no new issues

Review details

Commit: Commits 184ed57..11d236b
Model: claude-opus-4-8
Panel: correctness · robustness · design

@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: incremental · 42 files reviewed · no new issues

Review details

Commit: Commits 11d236b..aca56e0
Model: claude-opus-4-8
Panel: correctness · robustness · design

@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: incremental · 42 files reviewed · no new issues

Review details

Commit: Commits aca56e0..4ddb552
Model: claude-opus-4-8
Panel: correctness · robustness · design

@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: incremental · 43 files reviewed · 🟡 1 warning

Review details

Commit: Commits 4ddb552..65d8615
Model: claude-opus-4-8
Panel: correctness · robustness · design

Comment thread src/Template.php Outdated
if (null !== $declaration = $this->findDeclaredMacroName($name, $context)) {
[$declaredName, $templateName] = $declaration;
if ($declaredName !== $name) {
trigger_deprecation('twig/twig', '3.28', 'Testing whether the macro "%s" (defined in template "%s") is defined as "%s" is deprecated; macro names will be case-sensitive in Twig 4.0 and this test will return false.', $declaredName, $templateName, $name);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Warning — Wrong version attribution misleads users about when a deprecation/feature landed.

The version was bumped to 3.29.0 in this PR (Environment::VERSION = '3.29.0-DEV', CHANGELOG opens with # 3.29.0), yet the new deprecation strings and doc annotations still reference 3.28. trigger_deprecation('twig/twig', '3.28', ...) here — and the identical calls at lines 578 and 612, those in TwigMacro.php (168, 177, 182), plus the .. deprecated:: 3.28/.. versionadded:: 3.28 blocks and @final since Twig 3.28 on MacroNode — will attribute these deprecations/features to 3.28, but since 3.28.0 is already released they actually ship in 3.29.0. Update the version strings to 3.29.

@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: incremental · 43 files reviewed · no new issues

Review details

Commit: Commits 65d8615..a708108
Model: claude-opus-4-8
Panel: correctness · robustness · design

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.

2 participants