Skip to content

Navigation Menu

Sign in

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] Add support to only check first element of $data in ArrayDenormalizer #45337

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

Closed
wants to merge 1 commit into from
Closed

[Serializer] Add support to only check first element of $data in ArrayDenormalizer #45337

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
1 change: 1 addition & 0 deletions 1 src/Symfony/Component/Serializer/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ CHANGELOG
* Deprecate `ContextAwareDenormalizerInterface`, use `DenormalizerInterface` instead
* Deprecate `ContextAwareEncoderInterface`, use `EncoderInterface` instead
* Deprecate `ContextAwareDecoderInterface`, use `DecoderInterface` instead
* Add the ability to only check the first element in `ArrayDenormalizer`

6.0
---
Expand Down
16 changes: 14 additions & 2 deletions 16 src/Symfony/Component/Serializer/Normalizer/ArrayDenormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ class ArrayDenormalizer implements ContextAwareDenormalizerInterface, Denormaliz
{
use DenormalizerAwareTrait;

public function __construct(private bool $checkFirstElement = false)
{
}

/**
* {@inheritdoc}
*
Expand Down Expand Up @@ -69,8 +73,16 @@ public function supportsDenormalization(mixed $data, string $type, string $forma
throw new BadMethodCallException(sprintf('The nested denormalizer needs to be set to allow "%s()" to be used.', __METHOD__));
}

return str_ends_with($type, '[]')
&& $this->denormalizer->supportsDenormalization($data, substr($type, 0, -2), $format, $context);
if (!str_ends_with($type, '[]')) {
return false;
}

if ($this->checkFirstElement) {
$data = \is_array($data) ? $data : [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, if checkFirstElement is true and we are giving an int for example, we will convert the data to an empty array, which is quite odd to me.
What about returning false in that case?

$data = 0 === \count($data) ? null : reset($data);
CipherdevNL marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This basically means that we would call the nested denormalizers with $data === null which is as odd for two reasons:

  • Denormalizers are not used to getting called with null as data, so they might throw or return false.
  • If the original $data is an empty array, ArrayDenormalizer would not call denormalize() on the nested denormalizers at all.

Since we're attempting to remove odd behevior, I'd like to get it right this time. 😉

Suggestion: If the new mode is active and $data is an empty array, we should return true.

Copy link
Author

Choose a reason for hiding this comment

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

Totally agree of getting it right directly :). You're right, passing null might indeed be odd for denomalizers and may introduce other unwanted issues. And incase $data is empty this denormalizer will basically do nothing.

The only side effect would be, when an unsupported type is given and [] is passed by it will not throw an error. But when data is given (e.g. ["x", "y", "z"]) it will fail because it couldn't find a denormalizer for the type.

Think this side effect is only preventable by deprecating/ removing $data from the method signature, so we don't have the issue of "what to pass when the array is empty?". But I don't have a clear picture if $data in supportsDenormalization has a use case that can't be solved in the denormalize.

And incase we might want to introduce BC for 7.0 we can consider this option like already discussed in the issue itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally agree with @derrabus there.
Actually, if the array is empty, there is no need to check the first element as it doesn't exist.
Therefore, why not just resume the "default" behavior when the array is empty?

}

return $this->denormalizer->supportsDenormalization($data, substr($type, 0, -2), $format, $context);
}

/**
Expand Down
83 changes: 83 additions & 0 deletions 83 src/Symfony/Component/Serializer/Tests/Normalizer/ArrayDenormalizerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,89 @@ public function testSupportsNoArray()
)
);
}

public function testSupportsOtherDatatype()
{
$this->assertFalse(
$this->denormalizer->supportsDenormalization(
'83fd8e7c-61d4-4318-af88-fb34bd05e31f',
__NAMESPACE__.'\Uuid'
)
);

$denormalizer2 = new ArrayDenormalizer(true);
$denormalizer2->setDenormalizer($this->serializer);

$this->assertFalse(
$denormalizer2->supportsDenormalization(
'83fd8e7c-61d4-4318-af88-fb34bd05e31f',
__NAMESPACE__.'\Uuid'
)
);
}

public function testSupportsValidFirstArrayElement()
{
$denormalizer = new ArrayDenormalizer(true);
$denormalizer->setDenormalizer($this->serializer);

$this->serializer->expects($this->once())
->method('supportsDenormalization')
->with(['foo' => 'one', 'bar' => 'two'], ArrayDummy::class, 'json', [])
->willReturn(true);

$this->assertTrue(
$denormalizer->supportsDenormalization(
[
['foo' => 'one', 'bar' => 'two'],
['foo' => 'three', 'bar' => 'four'],
],
__NAMESPACE__.'\ArrayDummy[]',
'json'
)
);
}

public function testSupportsInValidFirstArrayElement()
{
$denormalizer = new ArrayDenormalizer(true);
$denormalizer->setDenormalizer($this->serializer);

$this->serializer->expects($this->once())
->method('supportsDenormalization')
->with(['foo' => 'one', 'bar' => 'two'], ArrayDummy::class, 'json', [])
->willReturn(false);

$this->assertFalse(
$denormalizer->supportsDenormalization(
[
['foo' => 'one', 'bar' => 'two'],
['foo' => 'three', 'bar' => 'four'],
],
__NAMESPACE__.'\ArrayDummy[]',
'json'
)
);
}

public function testSupportsNoFirstArrayElement()
{
$denormalizer = new ArrayDenormalizer(true);
$denormalizer->setDenormalizer($this->serializer);

$this->serializer->expects($this->once())
->method('supportsDenormalization')
->with($this->isNull(), ArrayDummy::class, 'json', [])
->willReturn(true);

$this->assertTrue(
$denormalizer->supportsDenormalization(
[],
__NAMESPACE__.'\ArrayDummy[]',
'json'
)
);
}
}

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