NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Serializer] Fixed`array_unique` on array of objects in`getAllowedAttributes`.#16450

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

ksm2 wants to merge5 commits intosymfony:masterfromksm2:ticket_16445

Closed

[Serializer] Fixed`array_unique` on array of objects in`getAllowedAttributes`.#16450

ksm2 wants to merge5 commits intosymfony:masterfromksm2:ticket_16445

Conversation

Copy link

Contributor

ksm2 commentedNov 3, 2015

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#16445
License	MIT
Doc PR	symfony/symfony-docs#16445

carsonbot added the Status: Needs Review label

Nov 3, 2015

Copy link

Contributor

jakzal commentedNov 3, 2015

Can you add a test case?

jakzal reviewed

Nov 3, 2015

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php Outdated

Copy link

Contributor

Choose a reason for hiding this comment

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

This could be inlined:

return$attributesAsString ?array_unique($allowedAttributes) :$allowedAttributes;

jakzal added Serializer Status: Needs Work and removed Status: Needs Review labels

Nov 3, 2015

ksm2 force-pushed theticket_16445 branch 3 times, most recently fromd311645 to3da6f29Compare

November 4, 2015 17:53

Copy link

Member

dunglas commentedNov 4, 2015

IMO$attributesAsString can just be deleted:#16445 (comment)

Copy link

Member

fabpot commentedNov 7, 2015

IIUC, thearray_unique can be removed altogether? If that's the case, let's do that.

Copy link

Member

dunglas commentedNov 7, 2015

It can with the metadata implementation provided in Symfony.

+1 for removing the call and adding a mention in the metadata interface that it's the responsibility of the implantation to return an uniquified list of attributes.

Copy link

Member

@fabpot I am sorry for the late response. I removed thearray_unique statement. Also I added a mention in theClassMetadataInterface that the stored attribute metadata is a set, not a list, containing not more than one object per attribute.

Copy link

Member

dunglas commentedDec 7, 2015

👍

Copy link

Member

fabpot commentedDec 7, 2015

@dunglas Which version should we merge this PR in?

Copy link

Member

dunglas commentedDec 7, 2015

2.7 IMO but not sure. I'll take a look and merge it.

Copy link

Member

fabpot commentedDec 8, 2015

Thank you @CornyPhoenix.

fabpot added a commit that referenced this pull request

Dec 8, 2015

bug#16450[Serializer] Fixedarray_uniqueon array of objects in `…

0b0070e

…getAllowedAttributes`. (CornyPhoenix)This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes#16450).Discussion----------[Serializer] Fixed `array_unique` on array of objects in `getAllowedAttributes`.| Q             | A| ------------- | ---| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#16445| License       | MIT| Doc PR        |symfony/symfony-docs#16445Commits-------6110bd9 [Serializer] Fixed  on array of objects in .

fabpot closed this

Dec 8, 2015