Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[serializer] extract normalizer tests to traits #30888

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.

Sign up for GitHub

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 1 commit into from
Apr 27, 2019
Merged

[serializer] extract normalizer tests to traits #30888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
89 changes: 79 additions & 10 deletions 89 src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,89 @@ abstract class AbstractNormalizer implements NormalizerInterface, DenormalizerIn
use ObjectToPopulateTrait;
use SerializerAwareTrait;

const CIRCULAR_REFERENCE_LIMIT = 'circular_reference_limit';
const OBJECT_TO_POPULATE = 'object_to_populate';
const GROUPS = 'groups';
const ATTRIBUTES = 'attributes';
const ALLOW_EXTRA_ATTRIBUTES = 'allow_extra_attributes';
const DEFAULT_CONSTRUCTOR_ARGUMENTS = 'default_constructor_arguments';
const CALLBACKS = 'callbacks';
const CIRCULAR_REFERENCE_HANDLER = 'circular_reference_handler';
const IGNORED_ATTRIBUTES = 'ignored_attributes';
/* constants to configure the context */

/**
* How many loops of circular reference to allow while normalizing.
*
* The default value of 1 means that when we encounter the same object a
* second time, we consider that a circular reference.
*
* You can raise this value for special cases, e.g. in combination with the
* max depth setting of the object normalizer.
*/
public const CIRCULAR_REFERENCE_LIMIT = 'circular_reference_limit';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about moving all these constants in the interface, and to add a builder object for better autocompletion in IDEs (exactly as done in HttpClient)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree that they should be in the interface. otherwise the interface is a lie. and we should define that when an option is not known by the normalizer implementation, it has to throw an exception. otherwise the behaviour is very undefined and e.g. typos are not spotted.

i also think we could use the OptionsResolver on them to validate, instead of the case-by-case validation we do at the moment (see #30907, #30950, its too easy to forget to validate things)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joelwurtz should we move the constants in this PR? i feel we should probably do that separately, also reviewing if they all stay the same or we want some different options and only keep some for legacy support.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like explained in #30818 we should have a global politic about context and where to put const of this context. I would be in favor of @dunglas proposal in order to be consistent with how others components works.

But it's clearly not in the scope of this PR. And this is something that should be done once we are satisified with the overall implementation. First let's do a good implementation, then we focus on DX for that, WDYT @dunglas ?

Copy link
Member

@dunglas dunglas Apr 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise the behaviour is very undefined and e.g. typos are not spotted

It’s not really doable because most likely the context will be passed to the Serializer instance, that will call many normalizers (another normalizer can be used for relations for instance).

+1 to move these constants in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we then make the serializer validate the context? e.g. by collecting from all its registered normalizers what options they support?

i am just very unhappy with these kind of unvalidated array configuration, as its super hard to debug when you make mistakes.

is the context supposed to also contain custom options for specific normalizers or is it a closed list of features? if it is closed, we could change from array to a value object that does the validation and defines what keys are possible. then we could do the validation once in that object and all normalizers can rely on only getting valid information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the context supposed to also contain custom options for specific normalizers or is it a closed list of features?

Yes, it is an extension point for custom normalizers and encoders too. It’s used (and sometimes abused) a lot by API Platform and many other tools using this component.

What we would to is to validate when using the new « context builder » as in HttpClient (when using this builder, we know the list of keys) and don’t validate anything (as currently) in the normalizers.
Another alternative would be to introduce a new parameter for « built-in » options that would be a class, but keep the context as an extension point (I’m less fond of this one because it will make the public API harder to learn).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we make the $context into a value object with explicit methods (and validation) for the known options, and a generic getExtra(string $name), setExtra(string $name, mixed $thing) method? this would make a good compromise to solve validation and the typo risk issue while keeping flexibility.

i dislike that every context validates the context parameters again, as they can be called repeatedly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is out of scope for this pull request. i don't have time for another pull request for it, but if somebody wants to do it, please do


/**
* Instead of creating a new instance of an object, update the specified object.
*
* If you have a nested structure, child objects will be overwritten with
* new instances unless you set DEEP_OBJECT_TO_POPULATE to true.
*/
public const OBJECT_TO_POPULATE = 'object_to_populate';

/**
* Only (de)normalize attributes that are in the specified groups.
*/
public const GROUPS = 'groups';

/**
* Limit (de)normalize to the specified names.
*
* For nested structures, this list needs to reflect the object tree.
*/
public const ATTRIBUTES = 'attributes';

/**
* If ATTRIBUTES are specified, and the source has fields that are not part of that list,
* either ignore those attributes (true) or throw an ExtraAttributesException (false).
*/
public const ALLOW_EXTRA_ATTRIBUTES = 'allow_extra_attributes';

/**
* Hashmap of default values for constructor arguments.
*
* The names need to match the parameter names in the constructor arguments.
*/
public const DEFAULT_CONSTRUCTOR_ARGUMENTS = 'default_constructor_arguments';

/**
* Hashmap of field name => callable to normalize this field.
*
* The callable is called if the field is encountered with the arguments:
*
* - mixed $attributeValue value of this field
* - object $object the whole object being normalized
* - string $attributeName name of the attribute being normalized
* - string $format the requested format
* - array $context the serialization context
*/
public const CALLBACKS = 'callbacks';

/**
* Handler to call when a circular reference has been detected.
*
* If you specify no handler, a CircularReferenceException is thrown.
*
* The method will be called with ($object, $format, $context) and its
* return value is returned as the result of the normalize call.
*/
public const CIRCULAR_REFERENCE_HANDLER = 'circular_reference_handler';

/**
* Skip the specified attributes when normalizing an object tree.
*
* This list is applied to each element of nested structures.
*
* Note: The behaviour for nested structures is different from ATTRIBUTES
* for historical reason. Aligning the behaviour would be a BC break.
*/
public const IGNORED_ATTRIBUTES = 'ignored_attributes';

/**
* @internal
*/
const CIRCULAR_REFERENCE_LIMIT_COUNTERS = 'circular_reference_limit_counters';
protected const CIRCULAR_REFERENCE_LIMIT_COUNTERS = 'circular_reference_limit_counters';

protected $defaultContext = [
self::ALLOW_EXTRA_ATTRIBUTES => true,
Expand Down
61 changes: 54 additions & 7 deletions 61 src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,60 @@
*/
abstract class AbstractObjectNormalizer extends AbstractNormalizer
{
const ENABLE_MAX_DEPTH = 'enable_max_depth';
const DEPTH_KEY_PATTERN = 'depth_%s::%s';
const DISABLE_TYPE_ENFORCEMENT = 'disable_type_enforcement';
const SKIP_NULL_VALUES = 'skip_null_values';
const MAX_DEPTH_HANDLER = 'max_depth_handler';
const EXCLUDE_FROM_CACHE_KEY = 'exclude_from_cache_key';
const DEEP_OBJECT_TO_POPULATE = 'deep_object_to_populate';
/**
* Set to true to respect the max depth metadata on fields.
*/
public const ENABLE_MAX_DEPTH = 'enable_max_depth';

/**
* How to track the current depth in the context.
*/
private const DEPTH_KEY_PATTERN = 'depth_%s::%s';

/**
* While denormalizing, we can verify that types match.
*
* You can disable this by setting this flag to true.
*/
public const DISABLE_TYPE_ENFORCEMENT = 'disable_type_enforcement';

/**
* Flag to control whether fields with the value `null` should be output
* when normalizing or omitted.
*/
public const SKIP_NULL_VALUES = 'skip_null_values';

/**
* Callback to allow to set a value for an attribute when the max depth has
* been reached.
*
* If no callback is given, the attribute is skipped. If a callable is
* given, its return value is used (even if null).
*
* The arguments are:
*
* - mixed $attributeValue value of this field
* - object $object the whole object being normalized
* - string $attributeName name of the attribute being normalized
* - string $format the requested format
* - array $context the serialization context
*/
public const MAX_DEPTH_HANDLER = 'max_depth_handler';
dbu marked this conversation as resolved.
Show resolved Hide resolved

/**
* Specify which context key are not relevant to determine which attributes
* of an object to (de)normalize.
*/
public const EXCLUDE_FROM_CACHE_KEY = 'exclude_from_cache_key';

/**
* Flag to tell the denormalizer to also populate existing objects on
* attributes of the main object.
*
* Setting this to true is only useful if you also specify the root object
* in OBJECT_TO_POPULATE.
*/
public const DEEP_OBJECT_TO_POPULATE = 'deep_object_to_populate';

private $propertyTypeExtractor;
private $typesCache = [];
Expand Down
16 changes: 16 additions & 0 deletions 16 src/Symfony/Component/Serializer/Tests/Fixtures/DeepObjectPopulateChildDummy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,20 @@ class DeepObjectPopulateChildDummy
public $foo;

public $bar;

// needed to have GetSetNormalizer consider this class as supported
public function getFoo()
{
return $this->foo;
}

public function setFoo($foo)
{
$this->foo = $foo;
}

public function setBar($bar)
{
$this->bar = $bar;
}
}
5 changes: 5 additions & 0 deletions 5 src/Symfony/Component/Serializer/Tests/Fixtures/MaxDepthDummy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,9 @@ public function getChild()
{
return $this->child;
}

public function getFoo()
{
return $this->foo;
}
}
13 changes: 0 additions & 13 deletions 13 src/Symfony/Component/Serializer/Tests/Normalizer/AbstractNormalizerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
use Symfony\Component\Serializer\Normalizer\ObjectNormalizer;
use Symfony\Component\Serializer\Tests\Fixtures\AbstractNormalizerDummy;
use Symfony\Component\Serializer\Tests\Fixtures\NullableConstructorArgumentDummy;
use Symfony\Component\Serializer\Tests\Fixtures\ProxyDummy;
use Symfony\Component\Serializer\Tests\Fixtures\StaticConstructorDummy;
use Symfony\Component\Serializer\Tests\Fixtures\StaticConstructorNormalizer;

Expand Down Expand Up @@ -99,18 +98,6 @@ public function testGetAllowedAttributesAsObjects()
$this->assertEquals([$a3, $a4], $result);
}

public function testObjectToPopulateWithProxy()
{
$proxyDummy = new ProxyDummy();

$context = [AbstractNormalizer::OBJECT_TO_POPULATE => $proxyDummy];

$normalizer = new ObjectNormalizer();
$normalizer->denormalize(['foo' => 'bar'], 'Symfony\Component\Serializer\Tests\Fixtures\ToBeProxyfiedDummy', null, $context);

$this->assertSame('bar', $proxyDummy->getFoo());
}

public function testObjectWithStaticConstructor()
{
$normalizer = new StaticConstructorNormalizer();
Expand Down
54 changes: 0 additions & 54 deletions 54 src/Symfony/Component/Serializer/Tests/Normalizer/AbstractObjectNormalizerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
use Symfony\Component\Serializer\Normalizer\ObjectNormalizer;
use Symfony\Component\Serializer\SerializerAwareInterface;
use Symfony\Component\Serializer\SerializerInterface;
use Symfony\Component\Serializer\Tests\Fixtures\DeepObjectPopulateChildDummy;
use Symfony\Component\Serializer\Tests\Fixtures\DeepObjectPopulateParentDummy;

class AbstractObjectNormalizerTest extends TestCase
{
Expand Down Expand Up @@ -163,58 +161,6 @@ public function testExtraAttributesException()
'allow_extra_attributes' => false,
]);
}

public function testSkipNullValues()
{
$dummy = new Dummy();
$dummy->bar = 'present';

$normalizer = new ObjectNormalizer();
$result = $normalizer->normalize($dummy, null, [AbstractObjectNormalizer::SKIP_NULL_VALUES => true]);
$this->assertSame(['bar' => 'present'], $result);
}

public function testDeepObjectToPopulate()
{
$child = new DeepObjectPopulateChildDummy();
$child->bar = 'bar-old';
$child->foo = 'foo-old';

$parent = new DeepObjectPopulateParentDummy();
$parent->setChild($child);

$context = [
AbstractObjectNormalizer::OBJECT_TO_POPULATE => $parent,
AbstractObjectNormalizer::DEEP_OBJECT_TO_POPULATE => true,
];

$classMetadataFactory = new ClassMetadataFactory(new AnnotationLoader(new AnnotationReader()));
$normalizer = new ObjectNormalizer($classMetadataFactory, null, null, new PhpDocExtractor());

$newChild = new DeepObjectPopulateChildDummy();
$newChild->bar = 'bar-new';
$newChild->foo = 'foo-old';

$serializer = $this->getMockBuilder(__NAMESPACE__.'\ObjectSerializerDenormalizer')->getMock();
$serializer
->method('supportsDenormalization')
->with($this->arrayHasKey('bar'),
$this->equalTo(DeepObjectPopulateChildDummy::class),
$this->isNull(),
$this->contains($child))
->willReturn(true);
$serializer->method('denormalize')->willReturn($newChild);

$normalizer->setSerializer($serializer);
$normalizer->denormalize([
'child' => [
'bar' => 'bar-new',
],
], 'Symfony\Component\Serializer\Tests\Fixtures\DeepObjectPopulateParentDummy', null, $context);

$this->assertSame('bar-new', $parent->getChild()->bar);
$this->assertSame('foo-old', $parent->getChild()->foo);
}
}

class AbstractObjectNormalizerDummy extends AbstractObjectNormalizer
Expand Down
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.