[GraphQL] Embedded entities support for mutations #1765
Conversation
alanpoulain
force-pushed
the
fix/graphql-embedded-entities
branch
2 times, most recently
from
f2ded97 to
24aeec8
Compare
| @@ -77,6 +77,10 @@ | |||
| <argument type="service" id="api_platform.item_data_provider" on-invalid="ignore" /> | |||
| <argument>%api_platform.allow_plain_identifiers%</argument> | |||
|
|
|||
| <call method="setItemNormalizer"> | |||
There was a problem hiding this comment.
We don't use setter injection in API Platform. Can you inject it directly in the constructor please?
There was a problem hiding this comment.
I've done it this way because the constructor of AbstractItemNormalizer has a lot of dependencies and I would need to copy it in this normalizer. But I can do it if you prefer.
There was a problem hiding this comment.
Yes I prefer to copy them. Please :)
| */ | ||
| public function denormalize($data, $class, $format = null, array $context = []) | ||
| { | ||
| return $this->itemNormalizer->denormalize($data, $class, $format, $context); |
There was a problem hiding this comment.
Can't you use parent:: directly? It feels weird to me to inject an item normalizer in another one.
There was a problem hiding this comment.
I can't because the default item normalizer is final. And I need it because I don't want to duplicate the code needed to update related existing resources.
There was a problem hiding this comment.
Remove the final keyword and add a @final PHPDoc annotation then. It will be cleaner IMHO.
| * | ||
| * @author Alan Poulain <[email protected]> | ||
| */ | ||
| class InputUnionType extends Type implements InputType, LeafType |
| */ | ||
| public function getTypes(): array | ||
| { | ||
| if (null === $this->types) { |
There was a problem hiding this comment.
I would use a guard clause (if (null !== $this->types) return...).
| public function getTypes(): array | ||
| { | ||
| if (null === $this->types) { | ||
| if (!isset($this->config['types'])) { |
There was a problem hiding this comment.
if ($types = $this->config['types'] ?? null) {
if (\is_callable($types)) {
$types = \call_user_func($this->config['types']);
}
}|
|
||
| $types = $this->getTypes(); | ||
| Utils::invariant( | ||
| !empty($types), |
There was a problem hiding this comment.
Can't empty can be removed? You can also inline this.
There was a problem hiding this comment.
I've copied it from here: https://github.com/webonyx/graphql-php/blob/master/src/Type/Definition/UnionType.php#L137
I think we can make a count instead. Do you agree?
| { | ||
| foreach ($this->getTypes() as $type) { | ||
| if ($type instanceof LeafType) { | ||
| if (null !== $parsed = $type->parseLiteral($valueNode)) { |
There was a problem hiding this comment.
can be merged with the previous line:
if ($type instanceof LeafType && null !== $parsed = $type->parseLiteral($valueNode))
alanpoulain
force-pushed
the
fix/graphql-embedded-entities
branch
from
24aeec8 to
952b56e
Compare
|
|
||
| Utils::assertValidName($config['name']); | ||
|
|
||
| Config::validate($config, [ |
There was a problem hiding this comment.
The Config class is deprecated, see webonyx/graphql-php#148.
| public function isValidValue($value): bool | ||
| { | ||
| foreach ($this->getTypes() as $type) { | ||
| if ($type instanceof LeafType) { |
There was a problem hiding this comment.
if ($type instanceof LeafType && $type->isValidValue($value)) {
return true;
}| public function isValidLiteral($valueNode): bool | ||
| { | ||
| foreach ($this->getTypes() as $type) { | ||
| if ($type instanceof LeafType) { |
There was a problem hiding this comment.
if ($type instanceof LeafType && $type->isValidLiteral($valueNode)) {
return true;
}
|
It should target master, it's a new feature. |
| /** | ||
| * @var array | ||
| */ | ||
| public $config; |
alanpoulain
force-pushed
the
fix/graphql-embedded-entities
branch
from
952b56e to
8c626bd
Compare
alanpoulain
force-pushed
the
fix/graphql-embedded-entities
branch
from
8c626bd to
69ca00d
Compare
|
@dunglas @meyerbaptiste Fixed. |
alanpoulain
force-pushed
the
fix/graphql-embedded-entities
branch
2 times, most recently
from
b20591a to
4b670a1
Compare
| } | ||
| } | ||
|
|
||
| throw new InvariantViolation(sprintf('Types in union cannot represent value: %s', Utils::printSafe($value))); |
| } | ||
| } | ||
|
|
||
| throw new Error(sprintf('Types in union cannot represent value: %s', Utils::printSafeJson($value))); |
| } | ||
|
|
||
| /** | ||
| * @throws InvariantViolation |
There was a problem hiding this comment.
Should be replaced by {@inheritdoc}.
alanpoulain
force-pushed
the
fix/graphql-embedded-entities
branch
from
4b670a1 to
b4415e5
Compare
|
@meyerbaptiste Done. |
|
Thank you very much @alanpoulain |
|
@alanpoulain let me ask you why this fix was not merged in production branch? |
|
Explanation about why this has been removed is here: #1886 |
|
@alanpoulain thanks. Looks like it's not solved yet. |
TODO:
Add a new InputUnionType to allow using either an IRI or data for relations.
It allows to modify embedded entities or to update a related existing resource (#1363).