[Form][FrameworkBundle][TwigBundle] Add Twig filter, form-type extension and improve service definitions for HtmlSanitizer #46335
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yceruto
reviewed
May 12, 2022
src/Symfony/Component/Form/Extension/HtmlSanitizer/Type/TextTypeHtmlSanitizerExtension.php
Outdated
Show resolved
Hide resolved
c9083f9 to
d0482b6
Compare
yceruto
approved these changes
May 12, 2022
tgalopin
approved these changes
May 13, 2022
There was a problem hiding this comment.
Thanks!
(small comment: my email is galopintitouan@gmail.com now 😊)
Kocal
reviewed
May 13, 2022
stof
requested changes
May 13, 2022
d0482b6 to
213c03a
Compare
e4dfa22 to
b7def25
Compare
…ion and improve service definitions for HtmlSanitizer
b7def25 to
0ea89e5
Compare
|
PR is now tested and ready, failures unrelated. Status: needs review |
fabpot
approved these changes
May 14, 2022
|
Thank you @nicolas-grekas. |
Merged
fabpot
added a commit
to twigphp/Twig
that referenced
this pull request
May 15, 2022
This PR was merged into the 2.x branch. Discussion ---------- Fix support for named closures As spotted by @stof on symfony/symfony#46335 (comment) Commits ------- 163f074 Fix support for named closures
wouterj
reviewed
May 15, 2022
| } | ||
|
|
||
| $sanitizers = $this->sanitizers; | ||
| $sanitizer = $options['sanitizer'] ?? $this->defaultSanitizer; |
There was a problem hiding this comment.
Is there a specific reason not to configure $this->defaultSanitizer as the default value in configureOptions()?
There was a problem hiding this comment.
Yes: to make it clear that null means "use the default value". I had removed the possibility to pass null before, and that made it impossible to say so.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR borrows the twig filter and the form-type extension from https://github.com/tgalopin/html-sanitizer-bundle/
Given the form builder (the input), it allows doing:
And on the template side (the output), it allows doing:
In order to be able to wire the corresponding services, I had to change the way html-sanitizer is wired by framework-bundle:
What we need here is a default name that we can rely on. By making the default name configurable, the current way to configure html_sanitizer makes it hard for other bundles (twig-bundle here) to know what the default name is. Eg here I would have to make the bundle read the config settings of framework-bundle. I solved this issue by making the default name non-configurable - aka I'm creating a convention: the default name is "default", and this "default" sanitizer is the one aliased to the html_sanitizer service, or to the non-named autowiring alias.
I'm submitting this PR to 6.1 because html-sanitizer is new. If we prefer merging this in 6.2, the updated service wiring must be merged into 6.1 at least.