-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[Serializer] split off an EncoderInterface and NormalizerInterface from SerializerInte #2530
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
b0daf35
split off an EncoderInterface and NormalizerInterface from Serializer…
lsmith77 58bd0f5
refactored the EncoderInterface
lsmith77 f8e2787
refactored Normalizer interfaces
lsmith77 d021dc8
refactored encoder handling to use the supports*() methods to determi…
lsmith77 2a6741c
typo fix
lsmith77 c3a711d
abstract class children should also implement dernormalization
lsmith77 078f7f3
more typo fixes
lsmith77 c3d6123
re-added supports(de)normalization()
lsmith77 351eaa8
require a (de)normalizer inside the (de)normalizable interfaces inste…
lsmith77 d811e29
CS fix
lsmith77 067242d
updated serializer tests to use the new interfaces
lsmith77 967531f
fixed various typos from the refactoring
lsmith77 cb495fd
added additional unit tests for deserialization
lsmith77 97389fa
use Serializer specific RuntimeException
lsmith77 72b9083
SerializerAwareNormalizer now only implements SerializerAwareInterface
lsmith77 0776b50
removed supports(De)Serializiation()
lsmith77 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
split off an EncoderInterface and NormalizerInterface from Serializer…
…Interface
- Loading branch information
commit b0daf3516f659b939728d3b289b46b2085690f96
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
<?php | ||
|
||
namespace Symfony\Component\Serializer; | ||
|
||
use Symfony\Component\Serializer\Encoder\EncoderInterface; | ||
|
||
/* | ||
* This file is part of the Symfony framework. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* This source file is subject to the MIT license that is bundled | ||
* with this source code in the file LICENSE. | ||
*/ | ||
|
||
/** | ||
* Defines the interface of the Encoder | ||
* | ||
* @author Jordi Boggiano <j.boggiano@seld.be> | ||
*/ | ||
interface EncoderInterface | ||
{ | ||
/** | ||
* Encodes data into the given format | ||
* | ||
* @param mixed $data data to encode | ||
* @param string $format format name | ||
* @return array|scalar | ||
*/ | ||
function encode($data, $format); | ||
|
||
/** | ||
* Decodes a string from the given format back into PHP data | ||
* | ||
* @param string $data data to decode | ||
* @param string $format format name | ||
* @return mixed | ||
*/ | ||
function decode($data, $format); | ||
|
||
/** | ||
* Checks whether the serializer can encode to given format | ||
* | ||
* @param string $format format name | ||
* @return Boolean | ||
*/ | ||
function supportsEncoding($format); | ||
|
||
/** | ||
* Checks whether the serializer can decode from given format | ||
* | ||
* @param string $format format name | ||
* @return Boolean | ||
*/ | ||
function supportsDecoding($format); | ||
|
||
/** | ||
* Get the encoder for the given format | ||
* | ||
* @return EncoderInterface | ||
*/ | ||
function getEncoder($format); | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
<?php | ||
|
||
namespace Symfony\Component\Serializer; | ||
|
||
use Symfony\Component\Serializer\Encoder\EncoderInterface; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This use statement is not used at all. and I find it confusing to have both |
||
|
||
/* | ||
* This file is part of the Symfony framework. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* This source file is subject to the MIT license that is bundled | ||
* with this source code in the file LICENSE. | ||
*/ | ||
|
||
/** | ||
* Defines the interface of the Normalizer | ||
* | ||
* @author Jordi Boggiano <j.boggiano@seld.be> | ||
*/ | ||
interface NormalizerInterface | ||
{ | ||
/** | ||
* Normalizes any data into a set of arrays/scalars | ||
* | ||
* @param mixed $data data to normalize | ||
* @param string $format format name, present to give the option to normalizers to act differently based on formats | ||
* @return array|scalar | ||
*/ | ||
function normalize($data, $format = null); | ||
|
||
/** | ||
* Denormalizes data into the given type. | ||
* | ||
* @param mixed $data | ||
* @param string $type | ||
* @param string $format | ||
* @return mixed | ||
*/ | ||
function denormalize($data, $type, $format = null); | ||
} |
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
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
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.
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 be an issue as the name is already used by the current interface. You would need to alias it.
And I don't like reusing the same interface short name as it is likely to be confusing.
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 also concerns me. not sure what better names there are though ..
EncoderManagerInterface
andNormalizerManagerInterface
?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.
btw, this interface could eventually extend both
Symfony\Component\Serializer\Encoder\EncoderInterface
andSymfony\Component\Serializer\Encoder\DecoderInterface
as it contains them.I guess we need to think a bit more about the way these interfaces are related (especially as the way the inheritance could be done is the opposite for the Normalizer part)
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.
well one is sort of a manager or dispatcher of sorts that will use the right encoder for the given format, while the other interface is for the encoders/decoders that do the actual work. for the manager it doesn't make sense to split up the encoding/decoding interfaces, since its generic code anyway, while for the encoder/decoders that do the work you might want to implement just one or the other depending on the format.
so imho the class structure is ok .. its essentially the naming that is iffy (as shown by my bogus use statement).
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 thing is, the
EncoderManagerInterface
could eventually extend from both other interfaces and add the 3 other methods. The question is "If someones asks for a DecoderInterface somewhere, does it make sense to allow passing the EncoderManagerInterface implementation (taking into account that it has the needed method) ?"If yes, this would be similar to what is done in the Routing component for
RouterInterface
,UrlMatcherInterface
andUrlGeneratorInterface
for instance.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 not about being aware of the encoder but about managing the different encoders. By looking at the different
*AwareInterface
in Symfony, it would be an interface with only one methodsetEncoder
.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.
true .. i guess i have the assumption that the
EncoderManagerInterface
will always delegate to "actual" Encoders which is also based on thegetEncoder
method. that being said .. maybe that method should be dropped since its too implementation specific.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.
Yeah, I guess this one should be dropped.
And maybe we should add the
supportsEncoding($format);
method in theEncoder\EncoderInterface
(as it may make sense here to know if an encoder supports a given format). Currently, the Serializer registers the encoder for a particular format, making it possible to do stupid things (a JsonEncoder for thexml
format ?) and making it harder to reuse the same encoder for several supported format as it requires registering it twice (JsonEncoder makes sense forjson
but also forjs
)Then, this interface would become unndeed as a delegating encoder is just a special implementation of the interface. See for instance how we have a ChainProvider in the Security component or a DelegatingTemplatingEngine for instance.
The serializer would then implement' interfaces:
SerializerInterface
,Encoder\EncoderInterface
,Encoder\DecoderInterface
(same change in it to add the method) andNormalizer\NormalizerInterface
(this would require adding thesupportNormalization
andsupportDenormalization
method in the Serializer)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.
btw, choosing the encoder based on a
support*
method defined in it would be more consistent with the way we do similar things in other parts of the framework.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 .. i like your suggestion and applied it .. what do you think now?