-
-
Notifications
You must be signed in to change notification settings - Fork 897
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): resource_class from context instead of entity class #6592
base: 3.3
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,7 +37,7 @@ | |
return; | ||
} | ||
|
||
$links = $this->getLinks($resourceClass, $operation, $context); | ||
$links = $this->getLinks($context['resource_class'] ?? $resourceClass, $operation, $context); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if |
||
|
||
if (!$links) { | ||
return; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe 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 |
||
|
||
if (!$links) { | ||
return; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the API Platform project. | ||
* | ||
* (c) Kévin Dunglas <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\Issue6590; | ||
|
||
use ApiPlatform\Doctrine\Orm\State\Options; | ||
use ApiPlatform\Metadata\ApiProperty; | ||
use ApiPlatform\Metadata\ApiResource; | ||
use ApiPlatform\Metadata\GraphQl\Query; | ||
use ApiPlatform\Metadata\GraphQl\QueryCollection; | ||
use ApiPlatform\Tests\Fixtures\TestBundle\Entity\Issue6590\Bar; | ||
use ApiPlatform\Tests\Fixtures\TestBundle\State\Issue6590\BarResourceProvider; | ||
|
||
#[ApiResource( | ||
shortName: 'Issue6590OrmBar', | ||
operations: [], | ||
graphQlOperations: [ | ||
new Query(), | ||
new QueryCollection(), | ||
], | ||
provider: BarResourceProvider::class, | ||
stateOptions: new Options(entityClass: Bar::class) | ||
)] | ||
class OrmBarResource | ||
{ | ||
#[ApiProperty(identifier: true)] | ||
public int $id; | ||
|
||
public string $name; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the API Platform project. | ||
* | ||
* (c) Kévin Dunglas <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\Issue6590; | ||
|
||
use ApiPlatform\Doctrine\Orm\State\Options; | ||
use ApiPlatform\Metadata\ApiProperty; | ||
use ApiPlatform\Metadata\ApiResource; | ||
use ApiPlatform\Metadata\GraphQl\Query; | ||
use ApiPlatform\Metadata\GraphQl\QueryCollection; | ||
use ApiPlatform\Tests\Fixtures\TestBundle\Entity\Issue6590\Foo; | ||
use ApiPlatform\Tests\Fixtures\TestBundle\State\Issue6590\FooResourceProvider; | ||
|
||
#[ApiResource( | ||
shortName: 'Issue6590OrmFoo', | ||
operations: [], | ||
graphQlOperations: [ | ||
new Query(), | ||
new QueryCollection(), | ||
], | ||
provider: FooResourceProvider::class, | ||
stateOptions: new Options(entityClass: Foo::class) | ||
)] | ||
class OrmFooResource | ||
{ | ||
#[ApiProperty(identifier: true)] | ||
public int $id; | ||
|
||
/** | ||
* @var OrmBarResource[] | ||
*/ | ||
public array $bars; | ||
|
||
public function addBar(OrmBarResource $bar): void | ||
{ | ||
$this->bars[] = $bar; | ||
} | ||
|
||
public function removeBar(OrmBarResource $bar): void | ||
{ | ||
unset($this->bars[array_search($bar, $this->bars, true)]); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the API Platform project. | ||
* | ||
* (c) Kévin Dunglas <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ApiPlatform\Tests\Fixtures\TestBundle\Entity\Issue6590; | ||
|
||
use Doctrine\ORM\Mapping as ORM; | ||
|
||
#[ORM\Entity()] | ||
#[ORM\Table(name: 'bar6590')] | ||
class Bar | ||
{ | ||
#[ORM\Id] | ||
#[ORM\Column(type: 'integer')] | ||
#[ORM\GeneratedValue(strategy: 'AUTO')] | ||
private int $id; | ||
|
||
#[ORM\Column] | ||
private string $name; | ||
|
||
#[ORM\ManyToOne(targetEntity: Foo::class, inversedBy: 'bars')] | ||
private ?Foo $foo = null; | ||
|
||
public function getId(): int | ||
{ | ||
return $this->id; | ||
} | ||
|
||
public function getName(): string | ||
{ | ||
return $this->name; | ||
} | ||
|
||
public function setName(string $name): self | ||
{ | ||
$this->name = $name; | ||
|
||
return $this; | ||
} | ||
|
||
public function getFoo(): ?Foo | ||
{ | ||
return $this->foo; | ||
} | ||
|
||
public function setFoo(?Foo $foo): self | ||
{ | ||
$this->foo = $foo; | ||
|
||
return $this; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the API Platform project. | ||
* | ||
* (c) Kévin Dunglas <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ApiPlatform\Tests\Fixtures\TestBundle\Entity\Issue6590; | ||
|
||
use Doctrine\Common\Collections\ArrayCollection; | ||
use Doctrine\Common\Collections\Collection; | ||
use Doctrine\ORM\Mapping as ORM; | ||
|
||
#[ORM\Entity] | ||
#[ORM\Table(name: 'foo6590')] | ||
class Foo | ||
{ | ||
#[ORM\Id] | ||
#[ORM\Column(type: 'integer')] | ||
#[ORM\GeneratedValue(strategy: 'AUTO')] | ||
private int $id; | ||
|
||
#[ORM\OneToMany(targetEntity: Bar::class, mappedBy: 'foo')] | ||
private Collection $bars; | ||
|
||
public function __construct() | ||
{ | ||
$this->bars = new ArrayCollection(); | ||
} | ||
|
||
public function getId(): int | ||
{ | ||
return $this->id; | ||
} | ||
|
||
/** | ||
* @return Collection<Bar> | ||
*/ | ||
public function getBars(): Collection | ||
{ | ||
return $this->bars; | ||
} | ||
|
||
/** | ||
* @param Collection<Bar> $bars | ||
*/ | ||
public function setBars(Collection $bars): self | ||
{ | ||
$this->bars = $bars; | ||
|
||
return $this; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the API Platform project. | ||
* | ||
* (c) Kévin Dunglas <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ApiPlatform\Tests\Fixtures\TestBundle\State\Issue6590; | ||
|
||
use ApiPlatform\Doctrine\Orm\Paginator; | ||
use ApiPlatform\Metadata\CollectionOperationInterface; | ||
use ApiPlatform\Metadata\Operation; | ||
use ApiPlatform\State\Pagination\TraversablePaginator; | ||
use ApiPlatform\State\ProviderInterface; | ||
use ApiPlatform\Tests\Fixtures\TestBundle\ApiResource\Issue6590\OrmBarResource; | ||
use ApiPlatform\Tests\Fixtures\TestBundle\Entity\Issue6590\Bar as BarEntity; | ||
|
||
class BarResourceProvider implements ProviderInterface | ||
{ | ||
public function __construct( | ||
private readonly ProviderInterface $itemProvider, | ||
private readonly ProviderInterface $collectionProvider, | ||
) { | ||
} | ||
|
||
public function provide(Operation $operation, array $uriVariables = [], array $context = []): object|array|null | ||
{ | ||
if ($operation instanceof CollectionOperationInterface) { | ||
$entities = $this->collectionProvider->provide($operation, $uriVariables, $context); | ||
\assert($entities instanceof Paginator); | ||
|
||
$resources = []; | ||
foreach ($entities as $entity) { | ||
$resources[] = $this->getResource($entity); | ||
} | ||
|
||
return new TraversablePaginator( | ||
new \ArrayIterator($resources), | ||
$entities->getCurrentPage(), | ||
$entities->getItemsPerPage(), | ||
$entities->getTotalItems() | ||
); | ||
} | ||
|
||
$entity = $this->itemProvider->provide($operation, $uriVariables, $context); | ||
|
||
return $this->getResource($entity); | ||
} | ||
|
||
protected function getResource(BarEntity $entity): OrmBarResource | ||
{ | ||
$resource = new OrmBarResource(); | ||
$resource->id = $entity->getId(); | ||
$resource->name = $entity->getName(); | ||
|
||
return $resource; | ||
} | ||
} |
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 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.
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.
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.
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.
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
Then the
entityClass
will be correct