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.

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

fix(doctrine): resource_class from context instead of entity class #6592

Open
wants to merge 3 commits into
base: 3.3
Choose a base branch
from
Open

fix(doctrine): resource_class from context instead of entity class #6592

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
350ecb3
fix(doctrine): resource_class from context instead of entity class
KaiGrassnick Sep 6, 2024
853e3ed
test(doctrine): add tests for links handler graphql query with dtos fix
KaiGrassnick Sep 7, 2024
09950a3
fix(doctrine): revert changes unrelated to the reported issue
KaiGrassnick Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Doctrine/Common/State/PersistProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

can you add a functional test with your stateOption? couldn't we get stateOptions->entityClass directly instead of using this context value?

Copy link
Author

Choose a reason for hiding this comment

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

@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.

Copy link
Author

@KaiGrassnick KaiGrassnick Sep 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO these lines are missing here:

core/src/Doctrine/Orm/State/ItemProvider.php

Lines 51 to 55 in a50e2d0

$entityClass = $operation->getClass();
if (($options = $operation->getStateOptions()) && $options instanceof Options && $options->getEntityClass()) {
$entityClass = $options->getEntityClass();
}

Then the entityClass will be correct

$reflectionProperties = $this->getReflectionProperties($data);

// TODO: the call to getReference is most likely to fail with complex identifiers
Expand Down
2 changes: 1 addition & 1 deletion src/Doctrine/Odm/State/LinksHandlerTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

if handleLinks receives the correct class, we don't need to use the $context here also.


if (!$links) {
return;
Expand Down
2 changes: 1 addition & 1 deletion src/Doctrine/Orm/State/LinksHandlerTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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.


if (!$links) {
return;
Expand Down
Loading