DataProducerExecutionTrait::executeDataProducer() ignores defined default values

[pmelab]

@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.

Originally posted by @Leksat in #1232 (comment)

[klausi]

Agreed, the ?bool $access is really confusing. Even the variable name should have been $check_access.

Yep, we should use just bool $access. We cannot use bool $access = TRUE because there are other required parameters afterwards.

[klausi]

From my understanding our "default_value" in the annotation is not supported by Drupal core's context system.

2 options:

1. we keep the ? optional operator on data producer parameters, since they will always receive NULL as default value.
2. we implement a better getContextValues() method in DataProducerPluginBase that handles default values.

I think option 2 should be doable.

[klausi]

Implemented option 2 in #1403

[edurenye]

If the MR has been merged, should we close this issue? Is all fixed?

[klausi]

yes, fixed!