-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Use the Twig guard tag #6656
Use the Twig guard tag #6656
Conversation
The test issues will be fixed when twigphp/Twig#4511 is released. |
@javiereguiluz the dev version of Twig 3.x should be working now thanks to twigphp/Twig#4575 |
0e8051b
to
80c601d
Compare
@stof I just updated this and I can confirm that your fix worked perfectly! 🙇 Thanks so much! |
5429b55
to
99f4f73
Compare
@@ -33,13 +33,14 @@ | |||
"symfony/security-bundle": "^5.4|^6.0|^7.0", | |||
"symfony/string": "^5.4|^6.0|^7.0", | |||
"symfony/translation": "^5.4|^6.0|^7.0", | |||
"symfony/twig-bridge": "^5.4,>=5.4.48|^6.0,>=6.4.16|^7.0,>=7.1.9", |
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.
This can be simplified as ^5.4.48|^6.4.16|^7.1.9
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'll make this change, thanks 🙏
<link rel="stylesheet" href="{{ (css_asset.preload ? ea_call_function_if_exists('preload', href, { as: 'style', nopush: css_asset.nopush }))|default(href) }}" | ||
{%- for attr, value in css_asset.htmlAttributes %} {{ attr }}="{{ value|e('html') }}"{% endfor %}> | ||
{% if css_asset.preload %} | ||
{% guard function preload %} |
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 the preload
function is not defined, shouldn't this fallback to the non-preload styles instead of not loading the CSS at all ?
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.
In most Symfony projects, yes. In this project, we always check (and throw an exception) if you don't have the needed code for some feature that you are using (see e.g.
EasyAdminBundle/src/Config/Asset.php
Lines 69 to 71 in da8f4e2
if (!class_exists('Symfony\\Component\\WebLink\\Link')) { | |
throw new \RuntimeException(sprintf('You are trying to preload an asset called "%s" but WebLink component is not installed in your project. Try running "composer require symfony/web-link"', $this->dto->getValue())); | |
} |
Then, if we always check first, why do we need this guard
tag? Because our checks are at runtime and Twig fails at compile-time if some function doesn't exist. That's why we need the compile-time protection that guard
brings. Thanks!
<script src="{{ (js_asset.preload ? ea_call_function_if_exists('preload', src, { as: 'script', nopush: js_asset.nopush }))|default(src) }}" {{ js_asset.async ? 'async' }} {{ js_asset.defer ? 'defer' }} | ||
{%- for attr, value in js_asset.htmlAttributes %} {{ attr }}="{{ value|e('html') }}"{% endfor %}></script> | ||
{% if js_asset.preload %} | ||
{% guard function preload %} |
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.
Same here
Twig 3.5 added the nice
guard
tag (https://twig.symfony.com/doc/3.x/tags/guard.html) to solve problems like the ones we have. So, let's use it.