Allow Markup to be marked as safe for escaping#4801
Conversation
stof
left a comment
stof
left a comment
There was a problem hiding this comment.
The {% set %} tag in capture mode should probably be updated to mark its Markup output as safe for the current autoescaping strategy rather than all.
But I'm not sure whether we can do that in 3.x or should wait for 4.0.
| private array $options; | ||
|
|
||
| public function __construct($content, $charset) | ||
| public function __construct($content, $charset, array $options = []) |
There was a problem hiding this comment.
I think we should pass an array of strategies there, not an array of options.
There was a problem hiding this comment.
I thought about that too, but was unsure whether there can be more options. Also, this would make the syntax kinda identical with functions and filters. But I'm totally fine with either way.
There was a problem hiding this comment.
I don't think the Markup value object need to be consistent in syntax with functions and filters. They have totally different purposes.
| public function escape($string, string $strategy = 'html', ?string $charset = null, bool $autoescape = false) | ||
| { | ||
| if ($autoescape && $string instanceof Markup) { | ||
| if ($autoescape && $string instanceof Markup && (null === $string->getSafe() || \in_array($strategy, $string->getSafe(), true))) { |
There was a problem hiding this comment.
This should probably take into account the case of strategies that are subset of others (html_attr is safe for html_attr_relaxed and html, and html_attr_relaxed is safe for html)
There was a problem hiding this comment.
I didn't know about that! Does that also apply to functions and filters? If my function [is_safe => 'html_attr'], does a foo.html.twig skip autoescaping? Could you point to any code to look at?
There was a problem hiding this comment.
See https://github.com/twigphp/Twig/blob/3.x/src/NodeVisitor/SafeAnalysisNodeVisitor.php which does the analysis for auto-escaping.
2 participants
Draft implementation of #4754
Let's keep the discussion on why and how to the original issue, but feel free to discuss the suggested code changes here 😊
/cc @Toflar