Skip to content

[TwigBridge][Validator] Add the Twig constraint and its validator #58805

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 28, 2025

Conversation

sfmok
Copy link
Contributor

@sfmok sfmok commented Nov 7, 2024

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues No
License MIT

Add a new constraint for validating Twig content (inspired from Yaml constraint)

@carsonbot carsonbot added this to the 7.2 milestone Nov 7, 2024
@sfmok sfmok changed the title Add the twig constraint and its validator [Validator] Add the twig constraint and its validator Nov 7, 2024
// Another syntax error example (unclosed variable)
['Hello {{ name', 'Unexpected token "end of template" ("end of print statement" expected) at line 1.', 1],
// Unknown filter error
['Hello {{ name | unknown_filter }}', 'Unknown "unknown_filter" filter at line 1.', 1],
Copy link
Member

Choose a reason for hiding this comment

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

Here, with the default environment, all the following would be invalid template, even if they are on most of the Symfony docs page.

{% form_theme form resources %}
{{ name | trans }}  
{% trans_default_domain domain %}
{{ object|serialize(format = 'json', context = []) }}
{{ text|humanize }}
{{ name | trans }}  

So ... what does "Twig valid" mean ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved by injecting the environment, as suggested by @derrabus #58805 (comment)

@carsonbot carsonbot changed the title [Validator] Add the twig constraint and its validator Add the twig constraint and its validator Nov 8, 2024
@carsonbot carsonbot changed the title Add the twig constraint and its validator [TwigBridge][Validator] Add the twig constraint and its validator Nov 8, 2024
@stof
Copy link
Member

stof commented Nov 8, 2024

What is the use case for this ?

@sfmok
Copy link
Contributor Author

sfmok commented Nov 8, 2024

What is the use case for this ?

@stof the constraint is intended to validate the Twig syntax of user-customized templates (e.g., emails, reports, widgets) to prevent syntax-related runtime errors.

@derrabus
Copy link
Member

derrabus commented Nov 9, 2024

The low-deps test failure appears to be caused by your changes.

// Another syntax error example (unclosed variable)
['Hello {{ name', 'Unexpected token "end of template" ("end of print statement" expected) at line 1.', 1],
// Unknown filter error
['Hello {{ name | unknown_filter }}', 'Unknown "unknown_filter" filter at line 1.', 1],
Copy link
Member

Choose a reason for hiding this comment

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

Yaml is a very defined syntax.

Here this is not the syntax that is validated, but the runtime Twig configuration.

And i still think it should not be exposed like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but the thrown exception indicates a syntax error.
See https://github.com/twigphp/Twig/blob/3.x/src/Error/SyntaxError.php


$value = (string) $value;

$prevErrorHandler = set_error_handler(static function ($level, $message, $file, $line) use (&$prevErrorHandler) {
Copy link
Member

Choose a reason for hiding this comment

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

A deprecated function is not valid ?

Copy link
Member

Choose a reason for hiding this comment

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

If you don't want deprecated Twig syntax to be used in your templates, why not? But we should probably make this behavior configurable.

Copy link
Member

Choose a reason for hiding this comment

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

As Twig is not aligned with Symfony release cycle, this seems a bit fragile to me, but why not with a flag, indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I've added the skipDeprecations option.

['Hello {{ name }}'],
['{% if condition %}Yes{% else %}No{% endif %}'],
['{# Comment #}'],
['Hello {{ "world" | upper }}'],
Copy link
Member

@fabpot fabpot Nov 10, 2024

Choose a reason for hiding this comment

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

Suggested change
['Hello {{ "world" | upper }}'],
['Hello {{ "world"|upper }}'],

And everywhere else where you're using the | operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sfmok sfmok force-pushed the twig-validator branch 4 times, most recently from f116b44 to aab17ec Compare November 10, 2024 17:13
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@sfmok sfmok requested a review from fabpot November 30, 2024 14:16
@@ -1,6 +1,11 @@
CHANGELOG
=========

7.2
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
7.2
7.3

7.2
---

* Added support for the new `twig` validator (from Twig Bridge)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Added support for the new `twig` validator (from Twig Bridge)
* Add support for the `twig` validator

@@ -5,6 +5,7 @@ CHANGELOG
---

* Deprecate passing a tag to the constructor of `FormThemeNode`
* Add the `Twig` constraint for validating Twig content
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Add the `Twig` constraint for validating Twig content
* Add the `Twig` constraint for validating Twig templates

@@ -5,6 +5,7 @@ CHANGELOG
---

* Deprecate passing a tag to the constructor of `FormThemeNode`
* Add the `Twig` constraint for validating Twig content
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved to a new 7.3 section.

@OskarStark OskarStark changed the title [TwigBridge][Validator] Add the twig constraint and its validator [TwigBridge][Validator] Add the Twig constraint and its validator Mar 26, 2025

#[HasNamedArguments]
public function __construct(
public string $message = 'This value is not valid Twig.',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public string $message = 'This value is not valid Twig.',
public string $message = 'This value is not a valid Twig template.',

@fabpot
Copy link
Member

fabpot commented Mar 28, 2025

Thank you @sfmok.

@fabpot fabpot merged commit 6ae4c85 into symfony:7.3 Mar 28, 2025
2 of 3 checks passed
@sfmok sfmok deleted the twig-validator branch April 6, 2025 19:01
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Apr 10, 2025
…fmok)

This PR was merged into the 7.3 branch.

Discussion
----------

[Validator] Add docs for bridge twig validator #20836

This pull request adds documentation for the new Twig Validator constraint introduced in the Twig Bridge.

Changes:
- Added a page to document the Twig validator constraint, including:
- Installation instructions for the symfony/twig-bridge package.
- Usage of the constraint in Symfony applications.
- Detailed options available, including `message` and `skipDeprecations`.
- Updated `index.rst` to include the new page under the "**Topics**" section for the **Twig Bridge.**

Related Issue:
[Issue #20836 - Add docs for Twig validator](#20836)

Related PR:
[Symfony PR #58805](symfony/symfony#58805) – Introduced the Twig validator constraint in the Twig Bridge.

Commits
-------

ac9e20c Add docs for bridge twig validator #20836
@stof
Copy link
Member

stof commented Apr 29, 2025

The default error message needs to be added in the translation files, so that it gets translated in the project.

fabpot added a commit that referenced this pull request Apr 30, 2025
…bbuh)

This PR was merged into the 6.4 branch.

Discussion
----------

[Validator] add translations for the Twig constraint

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

add missing translation file entries for #58805

Commits
-------

f12ba2e add translations for the Twig constraint
@fabpot fabpot mentioned this pull request May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants