Skip to content

Fix serialization when using interface as resource #2822

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

Jump to bottom
Merged
merged 1 commit into from
Jun 8, 2019
Merged

Fix serialization when using interface as resource #2822

merged 1 commit into from
Jun 8, 2019

Conversation

@teohhanhui
Copy link
Contributor

@teohhanhui teohhanhui commented May 25, 2019 •
edited
Loading

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

TODO:

  • Add tests

teohhanhui
teohhanhui commented May 25, 2019
View reviewed changes
src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php Outdated
$propertyMetadata = $this->transformLinkStatus($propertyMetadata, $normalizationGroups, $denormalizationGroups);
} catch (ResourceClassNotFoundException $e) {
// No need to check link status if related class is not a resource
// hack to allow non-resource classes (actually they should not be supported)
Copy link
Contributor Author

@teohhanhui teohhanhui May 25, 2019

Choose a reason for hiding this comment

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

If this hack is removed, all hell breaks loose, because we're using the PropertyMetadataFactory for input/output classes (DTOs). 😞

Copy link
Member

@dunglas dunglas May 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes we done it on purpose. It's a good move actually, it allows to retrieve the shape of any PHP structure, regardless if it is mapped with @ApiResource or not.

Copy link
Contributor Author

@teohhanhui teohhanhui May 25, 2019

Choose a reason for hiding this comment

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

Okay, I'll update the comment. 😎

Copy link
Contributor Author

@teohhanhui teohhanhui May 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

But actually, I can see many problems with using the property metadata system like this. It's simply not designed for non-resources, as you can see here. It'd give the wrong results (as in this case), which is arguably worse than not using it at all.

Copy link
Contributor Author

@teohhanhui teohhanhui May 27, 2019

Choose a reason for hiding this comment

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

@dunglas I hope this is okay for now: https://github.com/api-platform/core/compare/075222cd357ad018bc517e954ab46b5b4e9e90b3..731b45f5554d7ec832a33ed59fd1d1fac2230373

@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 1071f97 to 075222c Compare May 25, 2019 20:06
@teohhanhui teohhanhui changed the title Fix SerializerPropertyMetadataFactory readable/writable link value for resource with inheritance Fix SerializerPropertyMetadataFactory readable/writable link status for resource with inheritance May 25, 2019
@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 075222c to 731b45f Compare May 27, 2019 13:23
@teohhanhui teohhanhui mentioned this pull request May 27, 2019
Merged
@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 731b45f to a3fd33c Compare May 28, 2019 20:31
@teohhanhui teohhanhui changed the title Fix SerializerPropertyMetadataFactory readable/writable link status for resource with inheritance Fix serialization when using interface as resource May 28, 2019
@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch 3 times, most recently from 8330e42 to 8b5b129 Compare May 28, 2019 21:03
teohhanhui
teohhanhui commented May 28, 2019
View reviewed changes
@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 8b5b129 to f8e3011 Compare May 28, 2019 21:09
@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch from f8e3011 to 50221f5 Compare May 29, 2019 06:29
@teohhanhui teohhanhui requested a review from alanpoulain May 29, 2019 06:42
alanpoulain
alanpoulain reviewed Jun 4, 2019
View reviewed changes
alanpoulain
alanpoulain reviewed Jun 4, 2019
View reviewed changes
alanpoulain
alanpoulain reviewed Jun 4, 2019
View reviewed changes
src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php
$this->resourceMetadataFactory = $resourceMetadataFactory;
$this->serializerClassMetadataFactory = $serializerClassMetadataFactory;
$this->decorated = $decorated;
$this->resourceClassResolver = $resourceClassResolver;
Copy link
Member

@alanpoulain alanpoulain Jun 4, 2019

Choose a reason for hiding this comment

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

Deprecated if null?

Copy link
Contributor Author

@teohhanhui teohhanhui Jun 4, 2019

Choose a reason for hiding this comment

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

Hmm... We need to do this in many places. Will do this in a follow-up PR?

@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 50221f5 to 7bdb0f6 Compare June 4, 2019 16:03
@teohhanhui
Copy link
Contributor Author

teohhanhui commented Jun 4, 2019

Build failures should be fixed by #2841

We shall re-trigger the build after that's merged.

@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 7bdb0f6 to 6a64d36 Compare June 5, 2019 12:47
@teohhanhui
Copy link
Contributor Author

teohhanhui commented Jun 5, 2019

Build still failing, will be fixed by #2843

@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 6a64d36 to bba7e32 Compare June 7, 2019 14:45
@teohhanhui teohhanhui force-pushed the fix/serializer-readable-writable-link branch from bba7e32 to c9d9c58 Compare June 7, 2019 16:04
@teohhanhui teohhanhui merged commit 9044d7f into api-platform:2.4 Jun 8, 2019
@soyuka
Copy link
Member

soyuka commented Jun 10, 2019

nice :D @Nek- will be happy with this :D

@teohhanhui teohhanhui deleted the fix/serializer-readable-writable-link branch June 12, 2019 07:50
@bendavies
Copy link
Contributor

bendavies commented Jun 12, 2019

this has caused a regression for me and I can't seem to fix

{"type":"https:\/\/tools.ietf.org\/html\/rfc2616#section-10","title":"An error occurred","detail":"Object of type \u0022ClassA\u0022 does not match \u0022ClassB\u0022 resource class."}

I have an operation which returns a different class to resources the operation is defined on.

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Jun 12, 2019

@bendavies Can you open an issue?

@bendavies bendavies mentioned this pull request Jun 12, 2019
Closed
@bendavies
Copy link
Contributor

bendavies commented Jun 12, 2019

@teohhanhui done.

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

Reviewers

@alanpoulain alanpoulain alanpoulain left review comments

@dunglas dunglas dunglas approved these changes

@soyuka soyuka soyuka approved these changes

@meyerbaptiste meyerbaptiste Awaiting requested review from meyerbaptiste

Assignees

No one assigned

Labels

[RFR] Ready for Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@teohhanhui @soyuka @bendavies @dunglas @alanpoulain