-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Name converter support #13120
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
Conversation
*/ | ||
public function __construct(ClassMetadataFactory $classMetadataFactory = null) | ||
{ | ||
public function __construct( |
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.
The constructor signature should be a one liner I guess?
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 line will be longer than 80 characters.
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 know but as far as I know we don't wrap lines that exceeds 80 characters (even 120 characters).
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.
Ok changed.
dc3e274
to
7b9bb36
Compare
What is the intention with the serializer component? I think we should not just blindly rebuild all the features that the JMS serializer already has. IMO both serializers should join forces and make one good one that is also faster than the current JMS serializer. There is no point in just reinventing the wheel here. |
Hi @Tobion, My intention is not to blindly copy/reinvent JMS Serializer. I’ve already explained the need here: #12092 (comment) To summarize, I think we need:
The Symfony Serializer Compnent already has something similar to this feature (it’s why I deprecated it) but it has several drawbacks:
In fact, the Serializer Component will be lighter and more efficient by default with the Name Converter System than with the current implementation because it will not be enabled/registered without manual setting (so not loaded in memory). I think converting property names when serializing is a basic feature wanted by many people using a serializer. I've added a doc explaining some of these very common use cases: https://github.com/symfony/symfony-docs/pull/4692/files?diff=split Naming Strategies of JMSSerializer are similar but not as powerful. AFAIK the JMS implementation as the inverse drawback than the current Serializer Component implementation: it can rename attributes while serializing but not when deserializing. They are a lot of features I don’t want to see in the Serializer Component because they are redundant or will decrease its speed including:
However, JMSSerializer is a very powerful and well thinked library. I think we can study how it works and the experience with it to build something lighter but as powerful. One other thing I would like to see in the Symfony component is a more generic way than the callbacks system to handle special serialization (e.g.: being able to do something when encountering a special type like \DateTime). |
Thanks for the explanation. I agree having a more lightweight, MIT serializer that covers the JMS use-cases would be good.
No, it works both ways. |
Should be merged only after #13252 (and will need a rebase). |
00f5efb
to
e4f34a7
Compare
@fabpot rebased. |
* @param string $attributeName | ||
* @return string | ||
*/ | ||
protected function formatAttribute($attributeName) |
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.
That's a BC break.
e3f3980
to
fb65100
Compare
@fabpot BC break reverted. |
return ('.' === $match[1] ? '_' : '').strtoupper($match[2]); | ||
}, $attributeName); | ||
} | ||
trigger_error('The formatAttribute() method has been deprecated. Use Symfony\Component\Serializer\NameConverter\CamelCaseToSnakeCaseNameConverter instead.', E_USER_DEPRECATED); |
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.
Using __METHOD__
would be better than using formatAttribute()
(otherwise you don't know the method's 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.
and the wording actually is a bit different: http://symfony.com/doc/current/contributing/code/conventions.html#deprecations
@xabbuh fixed |
@dunglas You should use the actual version numbers instead of |
Oops :) |
7e40c6d
to
80191a3
Compare
fixed |
👍 |
}, $camelizedAttribute)); | ||
} | ||
|
||
$this->nameConverter = new CamelCaseToSnakeCaseNameConverter($attributes); |
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 will break if a NameConverter was passed in the constructor (you will removed the existing one entirely)
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 intended.
As NameConverter deprecate this method, both must not be used together. I'll add the throwing of an exception in that case (I thought it was done, but this is not the case).
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.
Exception added.
Please rebase this to fix conflicts (the logic common to the GetSetNormalizer and the PropertyNormalizer has been moved to their common parent class) |
80191a3
to
e7b1e86
Compare
Rebased. Thanks @stof for the review. |
e7b1e86
to
2cbd4ea
Compare
Tests fixed. |
@dunglas Looks like the tests are not fixed (at least, according to Travis) |
2cbd4ea
to
e14854f
Compare
@fabpot strange Travis issue. Commits squashed, now Travis has detected the changes. |
@dunglas Tests pass now on Travis as well apparently. |
The changelog of the component should be updated to mention the changes being done (adding the new feature and deprecating the old one). Otherwise 👍 |
Updated the changelog with new features of 2.6. I've also added a new feature of 2.6 missing from the changelog. Should I create a separate PR on the 2.6 branch for that? |
2.6.0 | ||
----- | ||
|
||
* added a new serializer: `PropertyNormalizer`. Like `GetSetMethodNormalizer`, | ||
this normalizer will map an object's properties to an array. | ||
* added circular references handling for `GetSetMethodNormalizer` |
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.
Indeed, you should submit a PR on 2.6 for this one.
Thank you @dunglas. |
This PR was merged into the 2.7 branch. Discussion ---------- [Serializer] Name converter support | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #12212 | License | MIT | Doc PR | symfony/symfony-docs#4692 This PR adds support for custom property naming strategies to the serializer and provides a built-in NameConverter using this new system: (CamelCase to underscore). It handles normalization and denormalization (convert `fooBar` to `foo_bar` when serializing, then from `foo_bar` to `fooBar` when deserializing). It also has a flag to convert only some attributes. The `setCamelizedAttributes()` is deprecated in favor of this new method (more flexible, allows to rename all attributes of a class and support deserialization) and now uses it internally. Commits ------- 86b84a5 [Serializer] Update changelog e14854f [Serializer] Name converter support
This PR was merged into the 2.7 branch. Discussion ---------- [Serializer] ObjectNormalizer | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT | Doc PR | not yet `PropertyAccessNormalizer` is a new normalizer leveraging the PropertyAccess Component. It is able to handle classes containing both public properties and properties only accessibles trough getters / setters / issers / hassers... As it extends `AbstractNormalizer`, it supports circular reference handling, name converters and existing object population. What do you think about making this new normalizer the default one as it's the most convenient to use and the most consistent with the behavior of other components. #13120, #13252 and #13255 need to be merged to make this PR working. Commits ------- 0050bbb [Serializer] Introduce ObjectNormalizer
This PR was merged into the 2.7 branch. Discussion ---------- [Serializer] Name Converter | Q | A | --------------- | --- | Doc fix? | no | New docs? | yes symfony/symfony#13120 | Applies to | 2.7 This is the documentation for the Serializer Name Converter feature. Commits ------- d335005 [Serializer] Fix CS 0442752 [Serializer] Acme things 57e6c94 [Serializer] Name Converter fixes 24d0320 [Serializer] Use firtName and snake_case 4ecc706 [Serializer] Name Converter corrections. b59b752 [Serializer] Name Converter
This PR adds support for custom property naming strategies to the serializer and provides a built-in NameConverter using this new system: (CamelCase to underscore).
It handles normalization and denormalization (convert
fooBar
tofoo_bar
when serializing, then fromfoo_bar
tofooBar
when deserializing). It also has a flag to convert only some attributes.The
setCamelizedAttributes()
is deprecated in favor of this new method (more flexible, allows to rename all attributes of a class and support deserialization) and now uses it internally.