fix(dataproducer): Populate context default values before resolving

[klausi]

Fix for #1233.

It is a bit unfortunate that Drupal core does not provide a good API function to get context values with default values populated, but not a big problem. We can implement that ourselves.

Now we could also update a lot of resolve() function signatures to remove the ? null type parameter, because now default values will always be passed.

Should we do it as part of this change or do we consider this an API break?

[Kingdutch]

Change looks good to me. I do think this is an API break that we may need to move to 5.x. I'm pretty sure I have default values in annotations somewhere which don't match what's in the code (which is silly, but that's not something that previously caused breakages).

[klausi]

That is a good point - if you have defined your default values wrong and they don't match the types on your resolver then there can be fatal errors.

We use quite a few dataproducers in Jobiqo and I did not see any problem in our automated testing, so probably this is a rare problem.

So I would risk it and push this to 4.x, with a warning in the release notes.

What do you think about changing the function signature of various dataproducer's resolve() methods? With this change we can remove a couple of ? operators of some types. I did not do that yet in this PR. I would consider the resolve() methods not part of the API surface of the graphql module, so we could risk that as well.

[Kingdutch]

I think in general I'm a bit more conservative with these kinds of changes. With all the modules we update in Open Social these kinds of subtle changes can add significant overhead which we tend to pay attention to with major updates, but with minor updates we generally start from the "I expect this still works the same and update mostly blindly" principle.

What do you think would be the impact of making this an opt-in flag in a minor version with a deprecation notice? Then we remove the flag in the next major version and default to the new behaviour?

if (Settings::get('graphql_context_defaults')) {
  $context = $this->getContextValuesWithDefaults();
}
else {
  @trigger_error("Not using defaults from context is deprecated in GraphQL 4.x and this behaviour will change in 5.0. Opt-in to the new behaviour by adding `$settings['graphql_context_defaults'] = TRUE;` to your settings.php file");
  $context = $this->getContextValues();
}

(above code is illustrative and not tested)

[klausi]

I'm not a fan of the setting as it makes it more complicated and harder to maintain, but it could work. We could have an update function for old installations that sets the config to false, then at least all new installations will get the new default.

I will check this approach.

[klausi]

Alright, this seems to work now. Can you review again and let me know if this is what you had in mind? Thanks!

[Kingdutch]

Yeah that’s awesome! Makes the minor release fully transparent (nothing happens) while allowing people to adopt the new behavior in preparation for the next major version. In the 5.0 release we can remove a bunch of code and the new behavior is the default.

Thanks! 🙌