-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
// 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], |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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)
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. |
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], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 }}'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
['Hello {{ "world" | upper }}'], | |
['Hello {{ "world"|upper }}'], |
And everywhere else where you're using the |
operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
f116b44
to
aab17ec
Compare
@@ -1,6 +1,11 @@ | |||
CHANGELOG | |||
========= | |||
|
|||
7.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7.2 | |
7.3 |
7.2 | ||
--- | ||
|
||
* Added support for the new `twig` validator (from Twig Bridge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Added support for the new `twig` validator (from Twig Bridge) | |
* Add support for the `twig` validator |
src/Symfony/Bridge/Twig/CHANGELOG.md
Outdated
@@ -5,6 +5,7 @@ CHANGELOG | |||
--- | |||
|
|||
* Deprecate passing a tag to the constructor of `FormThemeNode` | |||
* Add the `Twig` constraint for validating Twig content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Add the `Twig` constraint for validating Twig content | |
* Add the `Twig` constraint for validating Twig templates |
src/Symfony/Bridge/Twig/CHANGELOG.md
Outdated
@@ -5,6 +5,7 @@ CHANGELOG | |||
--- | |||
|
|||
* Deprecate passing a tag to the constructor of `FormThemeNode` | |||
* Add the `Twig` constraint for validating Twig content |
There was a problem hiding this comment.
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.
|
||
#[HasNamedArguments] | ||
public function __construct( | ||
public string $message = 'This value is not valid Twig.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public string $message = 'This value is not valid Twig.', | |
public string $message = 'This value is not a valid Twig template.', |
Thank you @sfmok. |
…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
The default error message needs to be added in the translation files, so that it gets translated in the project. |
…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
Add a new constraint for validating Twig content (inspired from Yaml constraint)