-
-
Notifications
You must be signed in to change notification settings - Fork 920
Trigger eager loading when using the attributes serializer option #1546
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
Conversation
132741a
to
748782b
Compare
748782b
to
fe80ac4
Compare
unset($context[AbstractNormalizer::ATTRIBUTES]); | ||
} | ||
|
||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cs is strange here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
|
||
if ( | ||
((!$inAttributes && false === $propertyMetadata->isReadableLink()) || false === $propertyMetadata->isReadable()) && | ||
false === $propertyMetadata->getAttribute('fetchEager', false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false === $propertyMetadata->getAttribute('fetchEager', false)
should not be needed here as it should've been tested https://github.com/dunglas/core/blob/master/src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php#L142
Did you start you PR on master? I can't recognize the replaced code which may have lead to weird copy/paste?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I let you handle that? I've changed this part (only the CS) and I'm not really comfortable with it.
TODO: