Skip to content
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

Doctrine Collections should be an array #37041

Closed
Gemorroj opened this issue Jun 1, 2020 · 10 comments
Closed

Doctrine Collections should be an array #37041

Gemorroj opened this issue Jun 1, 2020 · 10 comments

Comments

@Gemorroj
Copy link
Contributor

Gemorroj commented Jun 1, 2020

Symfony version(s) affected: 4.4.9

Description
After this change #36601 empty Doctrine Collections became objects instead of empty arrays

How to reproduce

use Doctrine\Common\Collections\ArrayCollection;
use Symfony\Component\Serializer\Encoder\JsonEncoder;
use Symfony\Component\Serializer\Serializer;

$serializer = new Serializer([], [new JsonEncoder()]);
$arr = new ArrayCollection();

echo $serializer->serialize($arr, 'json');

// 4.4.8 - []
// 4.4.9 - {}
@Gemorroj Gemorroj added the Bug label Jun 1, 2020
@Gemorroj Gemorroj changed the title Doctrine Collections should be array Doctrine Collections should be an array Jun 1, 2020
@soyuka
Copy link
Contributor

soyuka commented Jun 1, 2020

You should not expose ArrayCollection but use ArrayCollection->getValues instead, which will get you a real array (also fixes an index issue if you delete somthing from this ArrayCollection). ArrayCollection is an object and it's correct that it gets normalized to an object imo. See also api-platform/docs#504 and related.

@norkunas
Copy link
Contributor

norkunas commented Jun 1, 2020

@soyuka this also affected us somehow when orm paginator from api-platform returned zero results we started to get an empty object instead of empty array (for an application/json request).

@soyuka
Copy link
Contributor

soyuka commented Jun 1, 2020

I'd like to reproduce this @norkunas because it is indeed a bad behavior. I still believe that #36601 is legit or we should rollback this which actually causes the issue in the first place with \ArrayObject. I mean we can't on one hand allow empty objects to be normalized as empty objects (as they should) but have some kinds of objects be normalized as array (ArrayCollection/Paginator). According to the unit tests, an empty array is still encoded as empty array and the error should be best handled by the developer.

@norkunas
Copy link
Contributor

norkunas commented Jun 1, 2020

I agree that merged PR is a good one, but then api-platform should do the work to serialize paginator as empty array :)

@Gemorroj
Copy link
Contributor Author

Gemorroj commented Jun 1, 2020

@soyuka it's BC! And this is unexpected behavior and completely unusable in real world.
Symfony can give a setting for managing the behavior.

for example:

  • Doctrine Entity has ArrayCollection property (for relations)
  • Controller serialize the Entity with the property
  • Frontend expects an array

In symfony 4.4.8 it's works as expected.
But in 4.4.9 not.

DoctrineCollections should be implement JsonSerializable. But without it this change breaks a lot of production code.

@soyuka
Copy link
Contributor

soyuka commented Jun 1, 2020

What works as expected? Because normalizing an empty ArrayObject doesn't pre-4.4.9. I need to check if it worked pre-#28363 which then is also BC :/. I think that we should just deliberate on what we want the ObjectNormalizer to do on empty objects as pre-4.4.9 this is also "unexpected behavior":

use Doctrine\Common\Collections\ArrayCollection;
use Symfony\Component\Serializer\Encoder\JsonEncoder;
use Symfony\Component\Serializer\Serializer;

$serializer = new Serializer([], [new JsonEncoder()]);
$arr = new ArrayObject();

echo $serializer->serialize(['foo' => $arr], 'json');

@xabbuh
Copy link
Member

xabbuh commented Jun 1, 2020

Does #37049 fix the issue?

@soyuka
Copy link
Contributor

soyuka commented Jun 1, 2020

Does #37049 fix the issue?

Indeed, having the PRESERVE_EMPTY_OBJECTS to false keeps these objects as arrays. 👍 on the fix. (is this the default value btw?)

@Gemorroj
Copy link
Contributor Author

Gemorroj commented Jun 1, 2020

@xabbuh Yes, it works.

@dunglas
Copy link
Member

dunglas commented Jun 2, 2020

IMHO we should explicitly whitelist the old behavior for ArrayObject and instances of Doctrine Collections because they are designed to store lists (not maps) and it makes sense to always serialize them as JSON arrays (not objects).

@fabpot fabpot closed this as completed Jun 2, 2020
fabpot added a commit that referenced this issue Jun 2, 2020
… empty array objects (xabbuh)

This PR was merged into the 4.4 branch.

Discussion
----------

[Serializer] take into account the context when preserving empty array objects

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #37041
| License       | MIT
| Doc PR        |

Commits
-------

98fff21 take into account the context when preserving empty array objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants