8000 [Serializer] split off an EncoderInterface and NormalizerInterface from SerializerInte by lsmith77 · Pull Request #2530 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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 16 commits into from
Dec 14, 2011
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

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
lsmith77 committed Dec 11, 2011
commit b0daf3516f659b939728d3b289b46b2085690f96
63 changes: 63 additions & 0 deletions src/Symfony/Component/Serializer/EncoderInterface.php
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;
Copy link
Member

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.

Copy link
Contributor Author

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 and NormalizerManagerInterface ?

Copy link
Member

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 and Symfony\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)

Copy link
Contributor Author

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).

Copy link
Member

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 and UrlGeneratorInterface for instance.

Copy link
Member

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 method setEncoder.

Copy link
Contributor Author

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 the getEncoder method. that being said .. maybe that method should be dropped since its too implementation specific.

Copy link
Member

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 the Encoder\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 the xml format ?) and making it harder to reuse the same encoder for several supported format as it requires registering it twice (JsonEncoder makes sense for json but also for js)
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) and Normalizer\NormalizerInterface (this would require adding the supportNormalization and supportDenormalization method in the Serializer)

Copy link
Member

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.

Copy link
Contributor Author

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?


/*
* 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);
}
41 changes: 41 additions & 0 deletions src/Symfony/Component/Serializer/NormalizerInterface.php
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;
Copy link
Member

Choose a reason for hiding this comment

The 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 Symfony\Component\Serializer\NormalizerInterface and Symfony\Component\Serializer\Normalizer\NormalizerInterface, the first one being a subset of the other one.


/*
* 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);
}
2 changes: 1 addition & 1 deletion src/Symfony/Component/Serializer/Serializer.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
* @author Lukas Kahwe Smith <smith@pooteeweet.org>
*/
class Serializer implements SerializerInterface
class Serializer implements SerializerInterface, NormalizerInterface, EncoderInterface
{
protected $normalizers = array();
protected $encoders = array();
Expand Down
60 changes: 0 additions & 60 deletions src/Symfony/Component/Serializer/SerializerInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,43 +38,6 @@ function serialize($data, $format);
*/
function deserialize($data, $type, $format);

/**
* 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);

/**
* 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 serialize to given format
*
Expand All @@ -90,27 +53,4 @@ function supportsSerialization($format);
* @return Boolean
*/
function supportsDeserialization($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);
}
0