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

[Serializer] deserialize from xml: Fix a collection that contains the only one element #27326

Merged
merged 1 commit into from Jun 10, 2018

Conversation

Projects
None yet
6 participants
@webnet-fr
Contributor

webnet-fr commented May 21, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27279
License MIT
Doc PR

In xml when parent node (restaurants) contains several children nodes with the same tag (restaurant) it is clear that the children form a collection:

restaurants = {array} [1]
 restaurant = {array} [2]
  0 = {array} [2]
   name = "Some restaurant name"
   type = "Chinese"
  1 = {array} [2]
   name = "Another restaurant name"
   type = "Italian"

Afterwards the object denormalizer has no problem to create a collection of restaurants.

But when there is only one child (restaurant) the decoded normalized array will not contain a collection:

restaurants = {array} [1]
 restaurant = {array} [2]
  name = "Some restaurant name"
  type = "Chinese"

In this situation the object denormalizer threw unexpected exception. This PR modifies AbstractObjectNormalizer that is it will fill a collection containing the sole element properly.

@nicolas-grekas nicolas-grekas changed the title from deserialize from xml: Fix a collection that contains the only one ele… to [Serializer] deserialize from xml: Fix a collection that contains the only one element May 21, 2018

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 21, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented May 30, 2018

thank you @webnet-fr
there are some failures on 5.5 that look related. I think you're using deprecated mock functions for now.
Can you please have a look to make tests green?

@webnet-fr

This comment has been minimized.

Contributor

webnet-fr commented May 30, 2018

Thank you for your hint @nicolas-grekas (getMockBuilder should have been used).

There are still an strange error in tests for < PHP 7.0 which I am not sure how to deal with.
The list of arguments of AbstractNormalizer::instantiateObject as for Symfony 3.4 is:
(array &$data, $class, array &$context, \ReflectionClass $reflectionClass, $allowedAttributes/*, string $format = null*/)
But in tests it seems like string $format = null is uncommented which leads to errors for < PHP 7.0.

Could you have a look, please?

return true;
}
public function instantiateObject(array &$data, $class, array &$context, \ReflectionClass $reflectionClass, $allowedAttributes, string $format = null)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 30, 2018

Member

the "string" type hint is illegal here, branch 3.4 should work on PHP5.5

@nicolas-grekas

(failure unrelated)

@dunglas

This comment has been minimized.

Member

dunglas commented May 31, 2018

I'm not a fan of adding such quirks in the main normalizer hierarchy... But I've no better idea right now. Maybe should we introduce a specific XML normalizer instead?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 8, 2018

@dunglas: good enough as is?

@dunglas

dunglas approved these changes Jun 9, 2018

@fabpot

fabpot approved these changes Jun 10, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Jun 10, 2018

Thank you @webnet-fr.

@fabpot fabpot merged commit 1f346f4 into symfony:3.4 Jun 10, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 10, 2018

bug #27326 [Serializer] deserialize from xml: Fix a collection that c…
…ontains the only one element (webnet-fr)

This PR was squashed before being merged into the 3.4 branch (closes #27326).

Discussion
----------

[Serializer] deserialize from xml: Fix a collection that contains the only one element

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27279
| License       | MIT
| Doc PR        |

In xml when parent node (`restaurants`) contains several children nodes with the same tag (`restaurant`) it is clear that the children form a collection:

```
restaurants = {array} [1]
 restaurant = {array} [2]
  0 = {array} [2]
   name = "Some restaurant name"
   type = "Chinese"
  1 = {array} [2]
   name = "Another restaurant name"
   type = "Italian"
```

Afterwards the object denormalizer has no problem to create a collection of restaurants.

But when there is only one child (`restaurant`) the decoded normalized array will not contain a collection:

```
restaurants = {array} [1]
 restaurant = {array} [2]
  name = "Some restaurant name"
  type = "Chinese"
```

In this situation the object denormalizer threw unexpected exception. This PR modifies `AbstractObjectNormalizer` that is it will fill a collection containing the sole element properly.

Commits
-------

1f346f4 [Serializer] deserialize from xml: Fix a collection that contains the only one element
class SerializerCollectionDummy implements \Symfony\Component\Serializer\SerializerInterface, \Symfony\Component\Serializer\Normalizer\DenormalizerInterface
{
/** @var \Symfony\Component\Serializer\Normalizer\DenormalizerInterface */

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 11, 2018

Member

please review 24b6848 and report any mistake

This comment has been minimized.

@dunglas

dunglas Jun 11, 2018

Member

Looks good to me

This was referenced Jun 25, 2018

@nano-freelancer

This comment has been minimized.

nano-freelancer commented Sep 20, 2018

@webnet-fr as i wrote in issue 27279, provided above fix should be extended also for json-data

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment