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

fix(doctrine): denormalization of properties with embeds many that omit target document directive #4315

Merged
merged 6 commits into from
Jun 10, 2021

Conversation

wuchen90
Copy link
Contributor

Q A
Branch? 2.6
License MIT

According to doctrine/mongodb-odm docs, it's possible to embed many without declaring the target-document directive.

In api-platform, if we omit this directive, the deserialization won't work.

This fix makes the type guesser of MongoDB return null so other guessers will be used.

@alanpoulain
Copy link
Member

alanpoulain commented Jun 10, 2021

Hello,
Thanks for this fix, it seems OK to me.
The PR would be even better if there was a Behat test, do you think you can provide one?
You need to fix PHPStan too.

@wuchen90
Copy link
Contributor Author

The PR would be even better if there was a Behat test, do you think you can provide one?

Ok, I'll try my best to find my way in this tests folder.

@wuchen90
Copy link
Contributor Author

@alanpoulain
All done.

@@ -65,6 +65,10 @@ public function getTypes($class, $property, array $context = [])
if ($metadata->hasAssociation($property)) {
$class = $metadata->getAssociationTargetClass($property);

if (!\is_string($class) && null === $class) {
Copy link
Contributor Author

@wuchen90 wuchen90 Jun 10, 2021

Choose a reason for hiding this comment

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

This is some ugly attempt to please PHPStan and I don't know if this will work.
I'm not familiar with PHPStan so if someone knows how to fix it I'm all ears.

This problem here is the interface $metadata->getAssociationTargetClass returns a string in phpDoc, but implementation of it returns ?string.

See https://github.com/doctrine/mongodb-odm/blob/ccf26d9e023387e9aae86951186c043b3222fe70/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php#L1797
and https://github.com/doctrine/persistence/blob/79e97bcb1dfe3b242961253d81b8357a04e96ca1/lib/Doctrine/Persistence/Mapping/ClassMetadata.php#L128

The only solution I can figure out is:

Suggested change
if (!\is_string($class) && null === $class) {
if ($metadata instanceof \Doctrine\ODM\MongoDB\Mapping\ClassMetadata && null === $class) {

Copy link
Member

Choose a reason for hiding this comment

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

The best way to solve this is to add a PHPDoc on the returned variable to force PHPStan to consider it as ?string.
You can also launch PHPStan locally (vendor/bin/phpstan analyze .) in order to verify if it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! It works.
Thanks.

@wuchen90 wuchen90 changed the title fix(doctrine): denormalization of properties with embeds many which omit target document directive fix(doctrine): denormalization of properties with embeds many that omit target document directive Jun 10, 2021
@alanpoulain alanpoulain merged commit 64300b7 into api-platform:2.6 Jun 10, 2021
@alanpoulain
Copy link
Member

Thank you @wuchen90.

@wuchen90 wuchen90 deleted the fix-mongo branch June 14, 2021 07:26
LoicBoursin pushed a commit to LoicBoursin/core that referenced this pull request Aug 18, 2021
@nass59
Copy link

nass59 commented Sep 17, 2021

Hello, I have a use case where i need to store a document as a ReferenceOne, but it can be different types of documents.
And, since this PR there is an error on ReferenceOne when the targetDocument directive is not declared.

Property "content" on resource "App\Document\SomeDocument" is declared as a subresource, but its type could not be determined.

So is there a workaround to that ?
Thanks

@wuchen90
Copy link
Contributor Author

Hi @nass59
When the targetDocument directive is not there, the resolution of the object type is delegated to other guessers.
In your case, you need to use a discriminator map to help the internal of Symfony object serializer to guess the type.
See https://symfony.com/doc/current/components/serializer.html#serializing-interfaces-and-abstract-classes.
Hope this will help you.

@nass59
Copy link

nass59 commented Oct 7, 2021

Hi @wuchen90, it works very well ;) Thanks

vincentchalamon pushed a commit to vincentchalamon/core that referenced this pull request Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants