Fix entity reference language

[Leksat]

Fixes entity_reference, entity_reference_layout_revisions and entity_reference_revisions data producers. Now they return the translated entities in case if language is provided.

Based on @drupov's PR #974

[pmelab]

From a code perspective it seems fine. As I understand we will fall back to the source language if a language does not exist? Then it's up to the consumer to decide what to do. An option for "strict" language (only return items that exist in that language) could he helpful, but i would not put it in scope of this PR.

But we also need some tests for that. We only have one entity reference test right now:
https://github.com/drupal-graphql/graphql/blob/8.x-4.x/tests/src/Kernel/DataProducer/FieldTest.php

And it's weirdly named? Maybe we could use this as an opportunity to name it properly and add cases for this fix?

[klausi]

Agreed with @pmelab , tests would be good to cover the language. And we should use type hints in function signatures for all new code.

[Leksat]

@pmelab @klausi I guess this is out of scope, but there seems to be an issue with the data producer argument default values.

For example, the entity_reference plugin definition sets a default value for access argument:

graphql/src/Plugin/GraphQL/DataProducer/Field/EntityReference.php

Lines 46 to 50 in 6961eac

	* "access" = @ContextDefinition("boolean",
	* label = @Translation("Check access"),
	* required = FALSE,
	* default_value = TRUE
	* ),

So I would guess that the resolve method never gets null for $access argument. Right?

However, it is typehinted as ?bool:

graphql/src/Plugin/GraphQL/DataProducer/Field/EntityReference.php

Line 149 in 6961eac

public function resolve(EntityInterface $entity, $field, ?string $language, ?array $bundles, ?bool $access, ?AccountInterface $accessUser, ?string $accessOperation, FieldContext $context) {

Then, when we call DataProducerExecutionTrait::executeDataProducer() without providing access argument, the data producer receives access as null. So it does not check the access, even if this should be the default behavior.

I would suggest:

• Remove ? from data producer arguments having default values.
• Improve DataProducerExecutionTrait::executeDataProducer() to fill in default argument values.

[pmelab]

Does the incorrect default value behavior apply to actual execution within a schema request? Or is it scoped to DataProducerExecutionTrait::executeDataProducer(). We shouldn't introduce breaking changes by adding typehints.

[Leksat]

Does the incorrect default value behavior apply to actual execution within a schema request?

No.

With a real request, the default values are set from the plugin definition (here).

Or is it scoped to DataProducerExecutionTrait::executeDataProducer()

Yes. But this method is used a lot in tests.

[Leksat]

We shouldn't introduce breaking changes by adding typehints.

Well, yeah... If a data producer is extended from outside of the module, things might break.

[pmelab]

@klausi Do we consider a typehint change that will invalidate subclasses a breaking change?

I can't tell if a lot of people extend existing resolvers.

[klausi]

It depends if we consider data producer plugins as part of the API surface of graphql module or not. I'm not sure, when in doubt we should probably consider it as part of the stable API and we cannot change it.

I think we are lucky here and do not have a security issue since in real requests the access default value gets filled in correctly as you found out. Anyway, I think it makes sense to improve DataProducerExecutionTrait in the test to fill in default values if possible

[klausi]

Thanks a lot @Leksat ! I merged this now and we can follow-up with DataProducerExecutionTrait default value test improvements.