Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Add SanitizeDenormalizer for automatic sanitization#62117
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Conversation
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 PR misses the integration of this feature in FrameworkBundle.
src/Symfony/Component/Serializer/Normalizer/SanitizeDenormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/SanitizeDenormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/SanitizeDenormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/SanitizeDenormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Tests/Normalizer/SanitizeDenormalizerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Tests/Normalizer/SanitizeDenormalizerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Tests/Normalizer/SanitizeDenormalizerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…matic string sanitization
…ng in SanitizeDenormalizer and tests
2896194 to133b158Compare…g and update usage across denormalizer and tests
133b158 to32e14c7CompareThere 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
| returnfalse; | ||
| } | ||
| if (!class_exists($type)) { |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
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.