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] Serialize and deserialize from abstract classes#24375
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
[Serializer] Serialize and deserialize from abstract classes#24375
Uh oh!
There was an error while loading.Please reload this page.
Conversation
3476ea1 tod0d0264Compare| private$typeProperty; | ||
| private$typesMapping; | ||
| publicfunction__construct(string$typeProperty,array$typesMapping =array()) |
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.
Should be compatible with PHP 5.4 to target 3.4.
| return$this->typeProperty; | ||
| } | ||
| publicfunctionaddTypeMapping(string$type,string$typeClass) |
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.
Is it possible to remove this method? In the PropertyInfo component we used immutable objects.
| /** | ||
| * @author Samuel Roze <samuel.roze@gmail.com> | ||
| */ | ||
| finalclass ClassDiscriminatorResolver |
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.
It should implement an interface as it's not a value object (it has a behavior).
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protectedfunctioninstantiateObject(array &$data,$class,array &$context,\ReflectionClass$reflectionClass,$allowedAttributes,string$format =null) |
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.
Can't you move this in the parent class? It will make it easier to support for this feature in other normalizers.
dunglas commentedSep 29, 2017
Great, I like it a lot. Would fit perfectly to denormalize JSON-LD documents ( |
c472eaa tob3835d3Comparesroze commentedSep 29, 2017
@dunglas updated based on your requests and added the framework bundle bridge. |
cf3ee7d toeab4a89Comparesroze commentedSep 29, 2017
Can't get the bottom of this Travis issue for PHP 7+ 🤔 |
| */ | ||
| publicfunctionaddClassMapping($class,ClassDiscriminatorMapping$mapping) | ||
| { | ||
| if (isset($this->mapping[$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.
How about injecting the mappings through a constructor and making this verification at container compile time, not runtime?
| */ | ||
| publicfunctiongetMappingForClass($class) | ||
| { | ||
| if (isset($this->mapping[$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.
I thinkarray_key_exists can safely replaceisset here
theofidry commentedSep 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@sroze could you provide a sample of the problematic code? I do have similar relationships in my codebase and it works, because at the end of the day the class you need to serialize is not |
95313de to24c1e9eComparesroze commentedSep 29, 2017
@theofidry the tricky bit is the de-serialization, it would complain about not being able to instanciate an abstract class 😉 |
f779b20 to8caf193Comparejderusse commentedSep 29, 2017
Is this feature extensible to everything? and not only abstract classes. Would love to use Mapping could be done by using the basename of the Entity classname (serialize), and reverse mapping by browsing a list of known namespaces and chaking if the class name exists. WDYT? |
ogizanagi commentedSep 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I didn't look at this deep, but:
I agree this feature could be useful not only with abstract classes. |
| /** | ||
| * @author Samuel Roze <samuel.roze@gmail.com> | ||
| */ | ||
| finalclass ClassDiscriminatorMapping |
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.
we don't make classes "final" by default, eg for data objects. Is it the case here? If there is no special reason, I'd suggest to remove them.
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.
Yes, this is a data object. Happy to remove thefinal even tho I believe closed-first is the best approach 😜
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.
@nicolas-grekas removed the final 👍
sroze commentedSep 30, 2017
@jderusse yes, good point, I'll do this.
That's a very good point as well, this could potentially be automated using a
@ogizanagi The reason is that I started by exposing the
@ogizanagi I don't get the question. |
d668d4d to4a37a02Comparesroze commentedSep 30, 2017
Serializer's functional tests are the only one failing, with an exception with the newly introduced class: Any idea why@dunglas@nicolas-grekas ? |
| private$classMetadataFactory; | ||
| /** | ||
| * @param ClassMetadataFactoryInterface $classMetadataFactory |
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.
No need for this phpdoc
| <serviceid="serializer.property_accessor"alias="property_accessor" /> | ||
| <!-- Discriminator Map--> | ||
| <serviceid="serializer.mapping.class_discriminator_resolver"class="Symfony\Component\Serializer\Mapping\ClassDiscriminatorFromClassMetadata"> |
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 service must beprivate.
Can you aliasserializer.mapping.class_discriminator_resolver toClassDiscriminatorResolverInterface. It will allow to use autowiring.
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.
It is private, as it's in thedefaults. Added the alias.
| */ | ||
| publicfunction__construct(array$data) | ||
| { | ||
| if (!isset($data['typeProperty']) || !$data['typeProperty']) { |
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.
isset is faster andnull should throw an exception isn't it?
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.
Yes,null or a falsy contents should throw an exception, henceisset.
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 what'sempty made for
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.
empty fails if the key do not exists.
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.
@sroze : Hmm, no it doesn't 😅
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.
What I meant:
http://php.net/manual/en/function.empty.php
empty() does not generate a warning if the variable does not exist.
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.
fromhttp://php.net/manual/en/function.empty.php
No warning is generated if the variable does not exist. That means empty() is essentially the concise equivalent to !isset($var) || $var == false.
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.
Usedempty so we are all happy. Not sure about?? within aif and throwing an exception...
$this->typeProperty =$data['typeProperty'] ??thrownew ...;
??
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.
Oh. I was sure it was warninging, thanks ❤️
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.
Used it and added tests that proves it works 👍
| thrownewInvalidArgumentException(sprintf('Parameter "typeProperty" of annotation "%s" cannot be empty.',get_class($this))); | ||
| } | ||
| if (!isset($data['mapping']) ||empty($data['mapping'])) { |
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.
you can changeempty to! like in the previous test (or just useempty and remove the call toisset).
| $metadata =$this->classMetadataFactory->getMetadataFor($object); | ||
| if (null !==$metadata->getClassDiscriminatorMapping()) { | ||
| return$metadata->getClassDiscriminatorMapping(); |
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.
Storing the result in a local cache will improve performance (especially in the case where reflection is involved).
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.
Good point, added local cache bellow 👍 (the class metadata factory is already cached)
| */ | ||
| publicfunctiongetTypeForMappedObject($object) | ||
| { | ||
| if (null === ($mapping =$this->getMappingForMappedObject($object))) { |
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.
Parenthesis can be removed.
| parent::__construct($classMetadataFactory,$nameConverter); | ||
| $this->propertyTypeExtractor =$propertyTypeExtractor; | ||
| $this->classDiscriminatorResolver =$classDiscriminatorResolver ?: (null !==$classMetadataFactory ?newClassDiscriminatorFromClassMetadata($classMetadataFactory) :null); |
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.
We try to avoid imbricating ternaries (for readability). Can you switch to aif construct?
| protectedfunctiongetAttributeValue($object,$attribute,$format =null,array$context =array()) | ||
| { | ||
| if ($this->classDiscriminatorResolver &&$mapping =$this->classDiscriminatorResolver->getMappingForMappedObject($object)) { | ||
| if ($attribute ==$mapping->getTypeProperty()) { |
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.
You can merge bothif right?
| publicfunctiontestLoadDiscriminatorMap() | ||
| { | ||
| $classMetadata =newClassMetadata('Symfony\Component\Serializer\Tests\Fixtures\AbstractDummy'); |
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.
You can useAbstractDummy::class here too (recommended for new code)
| @@ -0,0 +1,15 @@ | |||
| <?php | |||
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.
Those files should me removed now isn't it?
9e12d44 tod21d2f9Comparesroze commentedOct 1, 2017
@jderusse this is now working with interfaces as well. |
sroze commentedOct 1, 2017
Test errors are normal as the functional tests are using |
| */ | ||
| protected$classDiscriminatorResolver; | ||
| publicfunction__construct(ClassMetadataFactoryInterface$classMetadataFactory =null,NameConverterInterface$nameConverter =null,PropertyTypeExtractorInterface$propertyTypeExtractor =null,ClassDiscriminatorResolverInterface$classDiscriminatorResolver =null) |
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.
Do we really want to keep exposing this$classDiscriminatorResolver argument? Configuring everything through metadata looks enough to me, isn't it?
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 would keep it. We may leverage this extension point with API Platform to remove a lot of code related toabstract classes handling. But we need to hook our own metadata system.
fabpot commentedOct 1, 2017
Moving this one to 4.1. There is no need to rush out such a new feature. |
sroze commentedOct 2, 2017
@fabpot changed the PR to be based on |
| * @var ClassMetadataFactoryInterface | ||
| */ | ||
| private$classMetadataFactory; | ||
| private$mappingForMappedObjectCache = []; |
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 don't think the short syntax for arrays has been accepted. Php-CS-fixer still has the long syntax specified in the config file
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.
But if we target PHP7+ we need to accept this :)
afurculitaOct 2, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 agree with you. But fabbot is still complaining. I think this rule should be removed from php_cs.dist and from fabbot, to permit new developments to use the short syntax. Would you like to open an issue / PR about this? At this point, according to this PR#22862, they try to avoid merge conflicts.
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.
#24396 👍
| * @param string $typeProperty | ||
| * @param array $typesMapping | ||
| */ | ||
| publicfunction__construct($typeProperty,array$typesMapping = []) |
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.
string can be added as typehint and the phpdoc removed
| * | ||
| * @return ClassDiscriminatorMapping|null | ||
| */ | ||
| publicfunctiongetMappingForClass(string$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.
A return type can be specified for each method
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.
We are targeting PHP 7.0, not 7.1, right? Can't add the return type when it can returnnull then 🤔
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.
Symfony 4 supports PHP >= 7.1.3
72ee16a to5a7f984Compare5a7f984 toa83ad3fCompareAdd the `discriminator_class_mapping` in the framework extensionDiscriminator map is compatible with interfaces
5271a72 to4c70e94Comparesroze commentedDec 3, 2017
Rebased on For you @symfony/deciders 👍 |
nicolas-grekas commentedDec 3, 2017
A failure on deps=low is almost never legitimate. Should be doubly checked. |
sroze commentedDec 3, 2017
@nicolas-grekas a new class used by FrameworkBundle's configuration (in the declared services). Should we add it only when the class exists or something? 🤔 |
nicolas-grekas commentedDec 3, 2017
class_exists if possible is best, instead of bumping the minimum version |
sroze commentedDec 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Good shout, just made the And it's green now. 💚 |
fabpot commentedDec 7, 2017
Thank you@sroze. |
…lasses (sroze)This PR was squashed before being merged into the 4.1-dev branch (closes#24375).Discussion----------[Serializer] Serialize and deserialize from abstract classes| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | ø| License | MIT| Doc PR | Not yetThis PR adds a feature in the Serializer: allow to serialize and de-serialize abstract classes. Such feature is especially useful when dealing with domain objects.# ExampleLet's take the example of the following objects:- `CodeRepository` defines a set of properties like `name` and `url`- `GitHubCodeRepository` and `BitBucketCodeRepository` extends from the abstract `CodeRepository` class and adds a few properties.- `Project` has a relation with a `codeRepository`, which has a type `CodeRepository`.At the moment, the serializer can't serialize/deserialize correctly this `Project` object has it doesn't know how to deal with this `CodeRepository` abstract object.This feature allows the serializer to deal with such situation. The `ObjectNormalizer` has now access to a `ClassDiscriminatorResolver` that knows, for a given abstract class:- Is the "type" property it needs to read/write to uniquely identify each sub-class- What's the name of the "type" for each sub-class mapping# Usage without Framework Bundle```php$discriminatorResolver = new ClassDiscriminatorResolver();$discriminatorResolver->addClassMapping(CodeRepository::class, new ClassDiscriminatorMapping('type', [ 'github' => GitHubCodeRepository::class, 'bitbucket' => BitBucketCodeRepository::class,]));$serializer = new Serializer(array(new ObjectNormalizer(null, null, null, null, $discriminatorResolver)), array('json' => new JsonEncoder()));$serialized = $serializer->serialize(new GitHubCodeRepository());// {"type": "github"}$repository = $serializer->unserialize($serialized, CodeRepository::class, 'json');// GitHubCodeRepository```# Usage with the Framework Bundle```yamlframework: serializer: discriminator_class_mapping: App\CodeRepository: type_property: type mapping: github: App\GitHubCodeRepository bitbucket: App\BitBucketCodeRepository```# Usage with Annotations/XML/YAML```phpuse Symfony\Component\Serializer\Annotation\DiscriminatorMap;/** * @DiscriminatorMap(typeProperty="type", mapping={ * "first"="Symfony\Component\Serializer\Tests\Fixtures\AbstractDummyFirstChild", * "second"="Symfony\Component\Serializer\Tests\Fixtures\AbstractDummySecondChild" * }) */abstract class AbstractDummy{ public $foo; public function __construct($foo = null) { $this->foo = $foo; }}```# TODO- [x] Working as standalone- [x] Working with the framework bundle- [x] Tests on mapping classesCommits-------4c6e05b [Serializer] Serialize and deserialize from abstract classes
sroze commentedDec 7, 2017 via email
I think we should do a blog post for this one. Javier, should we pair onthat? :) …On Thu, 7 Dec 2017 at 18:34, Fabien Potencier ***@***.***> wrote: Closed#24375 <#24375>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24375 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEXIVzy6LpTuTYyDEMUms0smEgFMpks5s-C_DgaJpZM4Pomri> . |
…n of interfaces and abstract classes (sroze, javiereguiluz)This PR was merged into the master branch.Discussion----------[Serializer] Add documentation about the (de)serialization of interfaces and abstract classesFixes#9011Add documentation forsymfony/symfony#24375Commits-------ac831b0 Minor fixes and some tweaks61b9c53 Add documentation about the (de)serialization of interfaces and abstract classes
Uh oh!
There was an error while loading.Please reload this page.
This PR adds a feature in the Serializer: allow to serialize and de-serialize abstract classes. Such feature is especially useful when dealing with domain objects.
Example
Let's take the example of the following objects:
CodeRepositorydefines a set of properties likenameandurlGitHubCodeRepositoryandBitBucketCodeRepositoryextends from the abstractCodeRepositoryclass and adds a few properties.Projecthas a relation with acodeRepository, which has a typeCodeRepository.At the moment, the serializer can't serialize/deserialize correctly this
Projectobject has it doesn't know how to deal with thisCodeRepositoryabstract object.This feature allows the serializer to deal with such situation. The
ObjectNormalizerhas now access to aClassDiscriminatorResolverthat knows, for a given abstract class:Usage without Framework Bundle
Usage with the Framework Bundle
Usage with Annotations/XML/YAML
TODO