[WIP] GraphQL Mutation support #1460
Conversation
alanpoulain
force-pushed
the
feature/graphql-mutations
branch
from
ca6200a to
5167847
Compare
| $request = Request::create($uri, $operation['method']); | ||
| $request->headers->set('Accept', 'application/json'); | ||
|
|
||
| $response = $this->httpKernel->handle($request, HttpKernelInterface::SUB_REQUEST); |
There was a problem hiding this comment.
This is weird but I get it, we don't actually have anything to handle object deletion, we just call remove() from Doctrine in the Doctrine's WriteListener: https://github.com/api-platform/core/blob/master/src/Bridge/Doctrine/EventListener/WriteListener.php#L62
It was OK for REST until now but it doesn't scale to GraphQL. Injecting HttpKernel here is bad design.
I suggest to do the following:
- Introduce a new
ObjectManagerInterfacein API Platform withdelete()andpersist()methods - Implement this interface in the Doctrine bridge to delegate to Doctrine's Object Manager (it will just be a proxy class)
- Use this new interface here and in
WriteListener
WDYT?
There was a problem hiding this comment.
Ok so you want to modify API Platform to avoid this issue then. Your suggestion seems good to me.
There was a problem hiding this comment.
Yes, it's a tiny, non-breaking change IMO.
There was a problem hiding this comment.
The existing Bridge\Doctrine\EventListener\WriteListener should be deprecated (and not registered anymore) and a new one (EventListener\WriteListener) using the new interface should be added.
There was a problem hiding this comment.
Can you please do this in another PR?
There was a problem hiding this comment.
Why not but this other PR must be merged before this one :)
| @@ -335,6 +373,10 @@ private function getOperations(ResourceMetadata $resourceMetadata, bool $isQuery | |||
| continue; | |||
| } | |||
|
|
|||
| if (Request::METHOD_PATCH === $operation['method']) { | |||
There was a problem hiding this comment.
You should probably whitelist methods here instead of blacklisting them, some other can be problematic (HEAD, OPTIONS...)
meyerbaptiste
force-pushed
the
feature/graphql-mutations
branch
from
5167847 to
bbf5047
Compare
|
Can we merge this one? |
|
I think @meyerbaptiste was working on a better way to do this, doesn't he? |
|
It's merged #1464, but it's lower level system. This PR can now be updated to use this new service. |
|
OK maybe I'll do that this weekend. |
alanpoulain
force-pushed
the
feature/graphql-mutations
branch
4 times, most recently
from
a7909e1 to
645a270
Compare
dunglas
force-pushed
the
feature/graphql-mutations
branch
from
645a270 to
a840669
Compare
dunglas
force-pushed
the
feature/graphql-mutations
branch
from
a52c25e to
8205168
Compare
|
Thanks @alanpoulain! This is still a WIP, but it's a big first step! |
* Delete mutation for an item * Put mutation for an item * Use object_to_populate * Basic create support * Fix and test create support * wip * Don't test composite identifiers support for now * Fix CS * Fix some tests * Fix CS * Fix tests
TODO: