[Serializer] Add SanitizeDenormalizer for automatic sanitization #62117
Conversation
stof
left a comment
There was a problem hiding this comment.
This PR misses the integration of this feature in FrameworkBundle.
src/Symfony/Component/Serializer/Normalizer/SanitizeDenormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/SanitizeDenormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/SanitizeDenormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/SanitizeDenormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Tests/Normalizer/SanitizeDenormalizerTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Tests/Normalizer/SanitizeDenormalizerTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Tests/Normalizer/SanitizeDenormalizerTest.php
Outdated
Show resolved
Hide resolved
…matic string sanitization
…ng in SanitizeDenormalizer and tests
…tion in container
2896194 to
133b158
Compare
…g and update usage across denormalizer and tests
133b158 to
32e14c7
Compare
stof
left a comment
There was a problem hiding this comment.
Note that support for scalar normalizers is currently broken in ObjectNormalizer, so you should probably wait for #58473 to be merged first.
Anyway, this PR will be for 8.1, not for 7.4 (as the feature freeze for 7.4 and 8.0 was beginning of October)
| ->tag('serializer.normalizer', ['built_in' => true, 'priority' => -915]) | ||
|
|
||
| ->set('serializer.denormalizer.sanitize', SanitizeDenormalizer::class) | ||
| ->args([service('service_container')]) |
There was a problem hiding this comment.
injecting the main DI container is a no-go. It should inject a service locator with the html sanitizer services instead (the main DI container won't be able to get those services anyway as they are not public)
| ->set('serializer.normalizer.number', NumberNormalizer::class) | ||
| ->tag('serializer.normalizer', ['built_in' => true, 'priority' => -915]) | ||
|
|
||
| ->set('serializer.denormalizer.sanitize', SanitizeDenormalizer::class) |
There was a problem hiding this comment.
this denormalizer needs to be removed from the config when the html-sanitizer component is not enabled (see how we handle other cross-component features in FrameworkExtension).
| continue; | ||
| } | ||
|
|
||
| foreach ($property->getAttributes(Context::class) as $attribute) { |
There was a problem hiding this comment.
This does not make any sense. You should read $context, not read attributes again on your own (this defeats all the benefits I listed in my previous review related to caching metadata or supporting all metadata formats)
| return false; | ||
| } | ||
|
|
||
| if (!class_exists($type)) { |
There was a problem hiding this comment.
In my previous review, I asked for this serializer to be a serializer applied on the string, not applied at the object level.
|
|
||
| foreach ($reflection->getProperties() as $property) { | ||
| $name = $property->getName(); | ||
| if (!isset($data[$name])) { |
There was a problem hiding this comment.
this is broken as it forgets about the support of many advanced features of the ObjectNormalizer (name converters, serialized names, etc...).
This is again something that would be solved automatically once you stop processing the object-level data in that normalizer in favor of making it a proper scalar normalizer.
This pull request introduces a new feature to the Symfony
Serializercomponent: Automatic sanitization of string and array of string properties during deserialization, using the newSanitizeDenormalizer.As it is already done in the Form Component, for a property to be sanitized, you must add the
sanitize_htmloption in the#[Context]attribute of that specific property. Without this option, the property will not be sanitized.On the top of this if you need to sanitize with a particular sanitizer you can use the second option
sanitizerto set the sanitizer you want to use, by default we use the default sanitizer.