Update GraphQL PHP dependency to v14.3.0 #1086

Kingdutch · 2020-09-14T13:24:34Z

Full changelog can be found here: https://github.com/webonyx/graphql-php/blob/master/CHANGELOG.md#v1430

Notable breaking changes in v14.0.0 that may affect user code:

Spec-compliance: Fixed ambiguity with null variable values and
default values
GraphQL\Deferred now extends
GraphQL\Executor\Promise\Adapter\SyncPromise
Renamed GraphQL\Error\Debug to GraphQL\Error\DebugFlag.
$positions in GraphQL\Error\Error constructor are not
nullable anymore. Same can be expressed by passing an empty
array.

Full changelog can be found here: https://github.com/webonyx/graphql-php/blob/master/CHANGELOG.md#v1430 Notable breaking changes in v14.0.0 that may affect user code: * Spec-compliance: Fixed ambiguity with null variable values and default values * `GraphQL\Deferred` now extends `GraphQL\Executor\Promise\Adapter\SyncPromise` * Renamed `GraphQL\Error\Debug` to `GraphQL\Error\DebugFlag`. * `$positions` in `GraphQL\Error\Error` constructor are not nullable anymore. Same can be expressed by passing an empty array.

A new deferred object used to be needed because `then` was called on and returned the wrapped promise of the `Deferred` object, which confused calling code. However, as of v14.0.0 of the graphql-php library the Deferred object extends the promise itself so `then` properly creates a new `Deferred` instance which will still work well with `instanceof` checks.

Kingdutch · 2020-09-14T13:59:56Z

The biggest reason I would like this update is that I had written code similar to the following snippet:

class Wrapper {

  public function metadata() {
    return $this->getData()->then(function ($data) {
       return ['metadata' => $data[0]->someData(); ];
    });
  }

  public function data() {
    return $this->getData();
  }

  protected function getData() {
    if ($this->cache === NULL) { 
      $this->cache = new Deferred(function () {
        return execute_expensive_cached_operation();
      });
    }
    
    return $this->cache;
  }
}

Intuitively this should work. However, before version v14.0.0 the Deferred::then returned the chained promise instead of a Deferred instance. This broke the resolver detection on whether a field result needed to be awaited, leading to cryptic client side errors (can not return NULL for non-nullable field).

After the update to v14.0.0 this code will work correctly because the new Deferred implementation will still return an instance of Deferred when calling Deferred::then. This also allows us to simplify some code in the GraphQL module although I could not find a lot of code that created nested Deferred values.

This commit introduces quite a bit of code. It implements the pagination based on the Relay connection spec: https://relay.dev/graphql/connections.htm It adds DataProducers and Wrappers that help in passing around and resolving the needed data and connection info. It implements the required TypedData to allow resolving of the PageInfo objects without additional classes or resolvers. This is done because the PageInfo should always be static. This is not done for Connections and Edges because they may contain more data based on the type of Edge or Connection that it is. It creates a base for entity query resolvers to make it easy to implement pagination for any type of entity within Open Social. The Open Social base schema is adjusted to use a custom resolver registry. This resolver registry implements logic to try and find field resolvers for any implemented interfaces in case a field resolver is not found on the type itself. This makes using the Connection and Edge interfaces a lot easier because the base resolvers don't need to be overwritten unless additional logic is needed. The code in this commit relies on a modified version of the Drupal graphql API that is compatible with v14.0.0 or newer of the graphql-php library. The changes can be found in drupal-graphql/graphql#1086

The `setDebug` method has been changed to `setDebugFlag`. In order to keep the user interface simple for people creating Drupal Server entities, this commit still keeps the binary "on/off" behaviour for debugging, using the new function. The only difference with the previous implementation is that `setDebug` in GraphQL would hide the flag but default to `1` (casting `(int)TRUE`). We now use the `2` value flag, which includes a backtrace with the error. This is more useful when debugging problems.

This commit introduces quite a bit of code. It implements the pagination based on the Relay connection spec: https://relay.dev/graphql/connections.htm It adds DataProducers and Wrappers that help in passing around and resolving the needed data and connection info. It implements the required TypedData to allow resolving of the PageInfo objects without additional classes or resolvers. This is done because the PageInfo should always be static. This is not done for Connections and Edges because they may contain more data based on the type of Edge or Connection that it is. It creates a base for entity query resolvers to make it easy to implement pagination for any type of entity within Open Social. The Open Social base schema is adjusted to use a custom resolver registry. This resolver registry implements logic to try and find field resolvers for any implemented interfaces in case a field resolver is not found on the type itself. This makes using the Connection and Edge interfaces a lot easier because the base resolvers don't need to be overwritten unless additional logic is needed. The code in this commit relies on a modified version of the Drupal graphql API that is compatible with v14.0.0 or newer of the graphql-php library. The changes can be found in drupal-graphql/graphql#1086

This commit updates code that checks for whether a promise needs to be resolved to accept `SyncPromise` instead of `Deferred`. Calling `then` on a `Deferred` instance will return a new `SyncPromise` instance that is the next promise in the chain. This is caused by the `SyncPromise` implementation calling `self`. This change allows a chained promise to be easily usable in resolvers. For example the following is now valid. ``` // in someclass public function resolve() { return $this->someMethodReturningDeffered()->then(function ($x) { // modify $x return $x; })->then(function ($y) { // modify $y return $y; }); } ``` The above code would previously lead to issues because a `SyncPromise` would be treated as a value instead of a deferred result. Before this change all values would have to be put in new `Deferred` instances which makes using promises a lot more difficult.

It was missed that the graphql-php library is installed manually during tests. So to test the new version it actually need to be required.

This commit introduces quite a bit of code. It implements the pagination based on the Relay connection spec: https://relay.dev/graphql/connections.htm It adds DataProducers and Wrappers that help in passing around and resolving the needed data and connection info. It implements the required TypedData to allow resolving of the PageInfo objects without additional classes or resolvers. This is done because the PageInfo should always be static. This is not done for Connections and Edges because they may contain more data based on the type of Edge or Connection that it is. It creates a base for entity query resolvers to make it easy to implement pagination for any type of entity within Open Social. The Open Social base schema is adjusted to use a custom resolver registry. This resolver registry implements logic to try and find field resolvers for any implemented interfaces in case a field resolver is not found on the type itself. This makes using the Connection and Edge interfaces a lot easier because the base resolvers don't need to be overwritten unless additional logic is needed. The code in this commit relies on a modified version of the Drupal graphql API that is compatible with v14.0.0 or newer of the graphql-php library. The changes can be found in drupal-graphql/graphql#1086

klausi

Thanks, I think this makes sense! It is a good idea to upgrade the library before our Drupal module 4.0 release, we don't have a stable release yet and I think we can make an exception for our API break promise because we are still in beta.

I have some small comments and I need to try this on our code base to check what other breaks I see.

klausi · 2020-10-25T09:00:32Z

src/Entity/Server.php

@@ -211,7 +212,7 @@ public function configuration() {
    // Create the server config.
    $registry = $plugin->getResolverRegistry();
    $server = ServerConfig::create();
-    $server->setDebug(!!$this->get('debug'));
+    $server->setDebugFlag($this->get('debug') ? DebugFlag::INCLUDE_TRACE : DebugFlag::NONE);


We should check if our debug variable with is_null() and then use DebugFlag::NONE. Otherwise use whatever integer is set in our setting. Also cast to int.

klausi · 2020-10-25T09:02:52Z

config/schema/graphql.schema.yml

@@ -12,7 +12,7 @@ graphql.graphql_servers.*:
      type: string
      label: 'Endpoint'
    debug:


I think we should also rename this to "debug_flag" to be in line with the lib. Makes it also more clear that this is then an integer.

Then we have a new property in the schema, we should check if that is a problem then.

klausi · 2020-10-25T09:08:47Z

Tests are failing because of Composer 2, will try to fix that first in another issue.

Kingdutch · 2020-10-25T09:17:51Z

I would be happy to update those parts of the config as well :) I initially didn’t do so to limit breaking changes to direct interaction with the 3rd party library and internal classes.

If we change the config schema and usage, we may also want to change the config form. An is_null check is probably not needed if the schema is non-nullable and the installation provides a default value :)

klausi · 2020-10-25T09:22:37Z

Right, good point! Yes, please update the debug flag and the config form.

We really need a test case for the config form, we have been avoiding that for a long time but would be super helpful here now.

klausi · 2020-10-25T09:47:56Z

Composer 2 problem fixed in #1100

klausi · 2020-11-02T14:33:53Z

We did a bit of internal testing for our decoupled clients and had quite a lot of test fails for our Behat test suite because of type strictness API breaks (Strict coercion of scalar types).

Similar on the frontend side, we need to update quite a few places in our TypeScript to handle the new strictness.

I'm afraid this will not be an easy update for GraphQL 4.x users. I still think we should do this as part of the 4.x branch, but I could be convinced to start a 5.x branch because of this. Interestingly we did not have any problems with Deferred as @Kingdutch mentioned, we struggle mostly with the int vs. string type strictness update.

Maybe we should do a quick poll here: please post a comment if you think we should only do this in a 5.x release and why. Otherwise I would do this update in 4.x and release a 4.0-beta2.

rthideaway · 2020-11-02T14:53:38Z

Personally I would do that in 4.x. Yes, it's a quite a big change, but it does not make too much sense to me to have separate 5.x branch which holds only webonyx library update with some other small changes. Seems kind of redundant. The strict types should be something we should all get familiar with as soon as possible. And sooner or later the update is inevitable anyway.

I also think not too many people are using 4.x right now, and the ones who are using it should be (hopefully) skilled enough to cut through the update.

Kingdutch · 2020-11-02T14:58:48Z

I think I'd agree with rthideaway here. I'm not sure how difficult it would be to maintain both 4.x and 5.x in parallel.

The mismatch between types in client code and server code can also cause some hard to debug bugs down the line, so it may be a good thing to force people to confront that. Investing the time now rather than in the future. It's just unfortunate that this wasn't enforceable from the get go.

pmelab · 2020-11-03T07:35:10Z

I would definitely favour upgrading within 4.x. Updating the frontend is not trivial, but if a type system is in place, it provides a clear path of action. And if not, it might not even be a problem because javascript won't care what scalar type it's dealing with?

I'm struggling to understand the implications for backend code. Do we have an example of a DataProducer that would break?

klausi · 2020-11-03T08:42:14Z

We have zero breaks in our backend code, I only know of the Deferred breaks that @Kingdutch mentioned above. No DataProducer that breaks. We had to rewrite our Behat graphql tests to also specify data types for fields (int vs string mostly), that is the biggest change apart to the frontend changes to fix the types (again string vs number).

klausi · 2020-11-03T17:04:04Z

So far everybody agrees that we can do this break in 4.x, let's aim for this to merge Monday Nov 9th.

@Kingdutch do you have time to clear up the debug_flag here this week?

klausi · 2020-11-05T14:47:12Z

I will look into this now and try to fix up the debug_flag.

Kingdutch · 2020-11-05T15:38:04Z

I happened to be already working no it :)

I've just pushed the commit with the requested changes. The only thing I have not been able to do is test the update hook since I didn't have an active server configuration to work on so this still needs testing.

klausi

Looks good to me, can you remove the devel use statement?

klausi · 2020-11-05T15:39:47Z

src/Form/ServerForm.php

@@ -12,7 +12,9 @@
 use Drupal\Core\Form\SubformState;
 use Drupal\Core\Plugin\PluginFormInterface;
 use Drupal\Core\Routing\RequestContext;
+use Drupal\devel\Twig\Extension\Debug;


devel should not be here :)

Ha I forgot I had an incorrect autocomplete from PHPStorm. Fixed.

The underlying library now allows to set the debugging configuration using a set of enum values forming a bitmask. The Server form is altered to show the individual options so that developers can select them. Additionally the use of `debug` in the code is changed to `debug_flag` to more clearly show how everything ties together. An update hook is included to ensure that if people are currently debugging they retain most of the information they expect.

klausi · 2020-11-05T16:11:30Z

Thanks, the update function does not work for me and enables debugging on a server where debugging was disabled. Will check why.

klausi · 2020-11-05T16:36:51Z

Ah, because I had a debug option override in our development settings.php file, that explains it.

Ok, then I think we are good here and can merge.

This commit introduces quite a bit of code. It implements the pagination based on the Relay connection spec: https://relay.dev/graphql/connections.htm It adds DataProducers and Wrappers that help in passing around and resolving the needed data and connection info. It implements the required TypedData to allow resolving of the PageInfo objects without additional classes or resolvers. This is done because the PageInfo should always be static. This is not done for Connections and Edges because they may contain more data based on the type of Edge or Connection that it is. It creates a base for entity query resolvers to make it easy to implement pagination for any type of entity within Open Social. The Open Social base schema is adjusted to use a custom resolver registry. This resolver registry implements logic to try and find field resolvers for any implemented interfaces in case a field resolver is not found on the type itself. This makes using the Connection and Edge interfaces a lot easier because the base resolvers don't need to be overwritten unless additional logic is needed. The code in this commit relies on a modified version of the Drupal graphql API that is compatible with v14.0.0 or newer of the graphql-php library. The changes can be found in drupal-graphql/graphql#1086

Kingdutch added 2 commits September 14, 2020 14:47

Kingdutch force-pushed the feature/graphql-php-v14 branch from 68124b6 to f726a5b Compare September 15, 2020 17:33

Kingdutch added 2 commits September 15, 2020 19:44

Install graphql-php 14.3 during tests

543ddde

It was missed that the graphql-php library is installed manually during tests. So to test the new version it actually need to be required.

Kingdutch force-pushed the feature/graphql-php-v14 branch from f726a5b to 543ddde Compare September 15, 2020 17:44

Kingdutch marked this pull request as ready for review September 16, 2020 07:42

Merge branch '8.x-4.x' into feature/graphql-php-v14

1ca331e

klausi reviewed Oct 25, 2020

View reviewed changes

klausi added the 4.x label Oct 25, 2020

Merge branch '8.x-4.x' into feature/graphql-php-v14

a180117

Merge branch '8.x-4.x' into feature/graphql-php-v14

4151ab5

klausi requested changes Nov 5, 2020

View reviewed changes

Kingdutch force-pushed the feature/graphql-php-v14 branch from ba424d0 to 2e3e5aa Compare November 5, 2020 15:41

klausi merged commit 466dece into drupal-graphql:8.x-4.x Nov 5, 2020

klausi mentioned this pull request Nov 9, 2020

Roadmap to 4.0 #936

Closed

6 tasks

Kingdutch deleted the feature/graphql-php-v14 branch November 9, 2020 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update GraphQL PHP dependency to v14.3.0 #1086

Update GraphQL PHP dependency to v14.3.0 #1086

Kingdutch commented Sep 14, 2020

Kingdutch commented Sep 14, 2020 •

edited

Loading

klausi left a comment

klausi Oct 25, 2020

klausi Oct 25, 2020

klausi commented Oct 25, 2020

Kingdutch commented Oct 25, 2020

klausi commented Oct 25, 2020

klausi commented Oct 25, 2020

klausi commented Nov 2, 2020 •

edited

Loading

rthideaway commented Nov 2, 2020

Kingdutch commented Nov 2, 2020 •

edited

Loading

pmelab commented Nov 3, 2020

klausi commented Nov 3, 2020

klausi commented Nov 3, 2020

klausi commented Nov 5, 2020

Kingdutch commented Nov 5, 2020

klausi left a comment

klausi Nov 5, 2020

Kingdutch Nov 5, 2020

klausi commented Nov 5, 2020

klausi commented Nov 5, 2020 •

edited

Loading

Update GraphQL PHP dependency to v14.3.0 #1086

Update GraphQL PHP dependency to v14.3.0 #1086

Conversation

Kingdutch commented Sep 14, 2020

Kingdutch commented Sep 14, 2020 • edited Loading

klausi left a comment

Choose a reason for hiding this comment

klausi Oct 25, 2020

Choose a reason for hiding this comment

klausi Oct 25, 2020

Choose a reason for hiding this comment

klausi commented Oct 25, 2020

Kingdutch commented Oct 25, 2020

klausi commented Oct 25, 2020

klausi commented Oct 25, 2020

klausi commented Nov 2, 2020 • edited Loading

rthideaway commented Nov 2, 2020

Kingdutch commented Nov 2, 2020 • edited Loading

pmelab commented Nov 3, 2020

klausi commented Nov 3, 2020

klausi commented Nov 3, 2020

klausi commented Nov 5, 2020

Kingdutch commented Nov 5, 2020

klausi left a comment

Choose a reason for hiding this comment

klausi Nov 5, 2020

Choose a reason for hiding this comment

Kingdutch Nov 5, 2020

Choose a reason for hiding this comment

klausi commented Nov 5, 2020

klausi commented Nov 5, 2020 • edited Loading

Kingdutch commented Sep 14, 2020 •

edited

Loading

klausi commented Nov 2, 2020 •

edited

Loading

Kingdutch commented Nov 2, 2020 •

edited

Loading

klausi commented Nov 5, 2020 •

edited

Loading