GraphQL + data provider refactoring: automatically add SQL join clauses #1619
Conversation
| */ | ||
| public function __construct(ManagerRegistry $managerRegistry, array $collectionExtensions = []) | ||
| { | ||
| $this->managerRegistry = $managerRegistry; | ||
| $this->collectionExtensions = $collectionExtensions; | ||
| } | ||
|
|
||
| public function supports(string $resourceClass, string $operationName = null): bool | ||
| public function supports(string $resourceClass, string $operationName = null, array $conext = []): bool |
There was a problem hiding this comment.
Didn't you mean $context?
| { | ||
| $options = null === $operationName ? [] : ['collection_operation_name' => $operationName]; | ||
| if (null === $resourceClass) { |
There was a problem hiding this comment.
What if $resourceClass is an empty string? I would prefer if (empty($resourceClass))
Same in all PR
There was a problem hiding this comment.
It would be a BC break ('' is currently allowed).
| @@ -53,6 +54,10 @@ | |||
| */ | |||
| public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, int $maxJoins = 30, bool $forceEager = true, RequestStack $requestStack = null, SerializerContextBuilderInterface $serializerContextBuilder = null, bool $fetchPartial = false, ClassMetadataFactoryInterface $classMetadataFactory = null) | |||
| { | |||
| if (null !== $this->serializerContextBuilder) { | |||
| @trigger_error('Passing an instance of "%s" is deprecated since version 2.2 and will be removed in 3.0. Use the "normalization_context" of the data provider\'s context instead.', E_USER_DEPRECATED); | |||
There was a problem hiding this comment.
Missing sprintf & argument
| @@ -53,6 +54,10 @@ | |||
| */ | |||
| public function __construct(PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, ResourceMetadataFactoryInterface $resourceMetadataFactory, int $maxJoins = 30, bool $forceEager = true, RequestStack $requestStack = null, SerializerContextBuilderInterface $serializerContextBuilder = null, bool $fetchPartial = false, ClassMetadataFactoryInterface $classMetadataFactory = null) | |||
| { | |||
| //if (null !== $this->serializerContextBuilder) { | |||
| // @trigger_error('Passing an instance of "%s" is deprecated since version 2.2 and will be removed in 3.0. Use the "normalization_context" of the data provider\'s context instead.', E_USER_DEPRECATED); | |||
| //} | |||
There was a problem hiding this comment.
You want to keep this dead code?
There was a problem hiding this comment.
No I'll drop it, I try to debug something.
| { | ||
| if (null === $resourceClass) { | ||
| throw new InvalidArgumentException('The "$resourceClass" parameter must not be null'); | ||
| } |
There was a problem hiding this comment.
Weird no? How is this possible and why do we change the interface to allow null? Especially if supports method does require this argument can't it be mandatory?
There was a problem hiding this comment.
It's only possible to defined optional parameters when overriding the signature of method defined in a parent interface. It's why.
There was a problem hiding this comment.
Sure. Once the parent interface argument becomes mandatory we will remove this test right?
There was a problem hiding this comment.
Indeed, but I'm not sure it is worth it to make the argument mandatory in a future version (pain to upgrade for almost no gain).
This PR is mainly a refactoring of the Data Providers to get rid of the dependency to HttpFoundation (it will also allow to simplify the JSONAPI support, and maybe to support PSR-7 later).
Thanks to this new decoupling, it's now possible to automatically add appropriate
JOINSQL clauses according to the selected fields in a GraphQL query.TODO: