fix(doctrine): resource_class from context instead of entity class #6592
Conversation
KaiGrassnick
force-pushed
the
fix/3.3-links-handler-graphql-query-with-dtos
branch
from
590d0d0 to
50085de
Compare
KaiGrassnick
force-pushed
the
fix/3.3-links-handler-graphql-query-with-dtos
branch
from
50085de to
350ecb3
Compare
KaiGrassnick
changed the title
| @@ -52,7 +52,7 @@ public function process(mixed $data, Operation $operation, array $uriVariables = | |||
| \assert(method_exists($manager, 'getReference')); | |||
| $newData = $data; | |||
| $identifiers = array_reverse($uriVariables); | |||
| $links = $this->getLinks($class, $operation, $context); | |||
| $links = $this->getLinks($context['resource_class'] ?? $class, $operation, $context); | |||
There was a problem hiding this comment.
can you add a functional test with your stateOption? couldn't we get stateOptions->entityClass directly instead of using this context value?
There was a problem hiding this comment.
@soyuka thanks for your reply. The stateOptions->entityClass could be null if not applied to an API Resource, thats why i opted for context usage.
I've just added a Functional Tests to the Branch.
There was a problem hiding this comment.
Question is, do we need the changes to the Persist Process generally? I've just changed it there as well to be consistent and the existing Test did not fail afterwards.
I'm sadly not that deep into API Platform to determine it right now.
For me, it was important to start working with the right Pattern, which always concludes the separation of Entities from View / Logic, but relying on existing Logic as much as Possible. Therefore i opted for the DTO Approach using stateOptions to still use the Doctrine Handler.
As it seems not a lot of people / projects are using the DTO Approach, specially with GraphQL, i would guess the Bug need some further investigation by People with more knowledge into API Platform.
Maybe someone can have a deeper look into this PR and Issue.
There was a problem hiding this comment.
IMHO these lines are missing here:
core/src/Doctrine/Orm/State/ItemProvider.php
Lines 51 to 55 in a50e2d0
Then the entityClass will be correct
* add test for bugfix
| @@ -37,7 +37,7 @@ private function handleLinks(Builder $aggregationBuilder, array $identifiers, ar | |||
| return; | |||
| } | |||
|
|
|||
| $links = $this->getLinks($resourceClass, $operation, $context); | |||
| $links = $this->getLinks($context['resource_class'] ?? $resourceClass, $operation, $context); | |||
There was a problem hiding this comment.
if handleLinks receives the correct class, we don't need to use the $context here also.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
| @@ -40,7 +40,7 @@ private function handleLinks(QueryBuilder $queryBuilder, array $identifiers, Que | |||
| $doctrineClassMetadata = $manager->getClassMetadata($entityClass); | |||
| $alias = $queryBuilder->getRootAliases()[0]; | |||
|
|
|||
| $links = $this->getLinks($entityClass, $operation, $context); | |||
| $links = $this->getLinks($context['resource_class'] ?? $entityClass, $operation, $context); | |||
There was a problem hiding this comment.
From what I understand the problem occurs only when it's an embed resource? I'm wondering though why this entityClass is wrong, are you able to trace that call and fix it in the class responsible from calling this? Basically at https://github.com/api-platform/core/blob/3.4/src/Doctrine/Orm/State/ItemProvider.php#L74 it should already be the correct class.
|
Stale bot made me look at this again, we may need to fix this and/or at least merge your tests, could you take a look at my comment? |
…default to old behavior of using entityClass
This PR fixes the issue stated in #6590.
In case of an Entity API Resource everything worked as expected, when using an DTO API Resource with stateOptions set to an Entity one would run into an Runtime Exception, as the LinksHandler could not fetch the Entity from the API Resource.
This could be fixed, by using the resource_class instead of the EntityClass from the EntityManager.