Repository: webonyx/graphql-php (php)

Issue:
Always call `loadPersistedQuery` if `queryId` was provided
Hey.

`graphql-php` conveniently provides means of implementing persisted queries - namely, it parses `queryId` from the request. The usual strategy is called Apollo automatic persisted queries - find the query in cache, return if found, cache if not found. This is implemented by all major GraphQL libraries: Apollo Server, Hot Chocolate, ExpediaGroup/graphQL. 

The process is described in detail here: https://www.apollographql.com/docs/apollo-server/performance/apq/

This would have worked in `graphql-php` too, but there's a small stopper which prevents us from doing so: https://github.com/webonyx/graphql-php/blob/master/src/Server/Helper.php#L258. If `queryId` and `query` are both provided (as is the case when a query by ID wasn't initially found), then persisted query loader isn't executed, meaning there's no way to actually save the query into cache.

This can be worked around in userland by overwriting `Helper::promiseToExecuteOperation`, but that isn't an elegant solution to what seems to be just a precaution of the past. Instead, we can simply change the condition to NOT check for `query` at all, and instead always use `persistedQueryLoader` whenever a `queryId` is provided. 

Would this kind of change be accepted? If so, I'll PR the change.

Pull request description:
Call persistedQueryLoader even when query was passed
Closes #1370.

This partially reverses #790. That PR was _supposed_ to fix the support of [Automatic persisted queries](https://www.apollographql.com/docs/apollo-server/performance/apq/), but it has only partially done so. By adding the `$query === null` condition, it contradicts the flow proposed by Apollo and @cwdt in their original PR: 

```
3. If the backend doesn't recognize the hash, it will ask the client to send the hash and the query text so it can store them mapped together for future lookups. During this request, the backend will also fulfill the data request.
```

The important part here is `so it can store them mapped together for future lookups` - this is currently not possible because `persistedQueryLoader` is not called when both `queryId` and `query` were passed in a request.

Since `webonyx/graphql-php` parses Apollo-style automatic persisted query IDs (see https://github.com/webonyx/graphql-php/blob/master/src/Server/OperationParams.php#L123), it makes sense to allow them to be implemented in userland.

Although this is technically a fix of the previous PR, I've marked it as a "change" in the changelog because it might be a breaking change for some.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 696e4e14..97ef3107 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,10 @@ You can find and compare releases at the [GitHub release page](https://github.co
 
 ## Unreleased
 
+### Changed
+
+- Allow sending both `query` and `queryId`, let `persistedQueryLoader` handle it
+
 ## v15.3.0
 
 ### Added
diff --git a/docs/executing-queries.md b/docs/executing-queries.md
index e7813bf7..234f179d 100644
--- a/docs/executing-queries.md
+++ b/docs/executing-queries.md
@@ -96,19 +96,19 @@ PSR-7 is useful when you want to integrate the server into existing framework:
 
 ### Server configuration options
 
-| Argument             | Type                                                                        | Notes                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           |
-| -------------------- | --------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| schema               | [`Schema`](class-reference.md#graphqltypeschema)                            | **Required.** Instance of your application [Schema](schema-definition.md)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       |
-| rootValue            | `mixed`                                                                     | Any value that represents a root of your data graph. It is passed as the 1st argument to field resolvers of [Query type](schema-definition.md#query-and-mutation-types). Can be omitted or set to null if actual root values are fetched by Query type itself.                                                                                                                                                                                                                                                                                                                                                                                  |
-| context              | `mixed`                                                                     | Any value that holds information shared between all field resolvers. Most often they use it to pass currently logged in user, locale details, etc.<br><br>It will be available as the 3rd argument in all field resolvers. (see section on [Field Definitions](type-definitions/object-types.md#field-configuration-options) for reference) **graphql-php** never modifies this value and passes it _as is_ to all underlying resolvers.                                                                                                                                                                                                        |
-| fieldResolver        | `callable`                                                                  | A resolver function to use when one is not provided by the schema. If not provided, the [default field resolver is used](data-fetching.md#default-field-resolver).                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |
+| Argument             | Type                                                                        | Notes                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                          |
+| -------------------- | --------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| schema               | [`Schema`](class-reference.md#graphqltypeschema)                            | **Required.** Instance of your application [Schema](schema-definition.md)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      |
+| rootValue            | `mixed`                                                                     | Any value that represents a root of your data graph. It is passed as the 1st argument to field resolvers of [Query type](schema-definition.md#query-and-mutation-types). Can be omitted or set to null if actual root values are fetched by Query type itself.                                                                                                                                                                                                                                                                                                                                                                                 |
+| context              | `mixed`                                                                     | Any value that holds information shared between all field resolvers. Most often they use it to pass currently logged in user, locale details, etc.<br><br>It will be available as the 3rd argument in all field resolvers. (see section on [Field Definitions](type-definitions/object-types.md#field-configuration-options) for reference) **graphql-php** never modifies this value and passes it _as is_ to all underlying resolvers.                                                                                                                                                                                                       |
+| fieldResolver        | `callable`                                                                  | A resolver function to use when one is not provided by the schema. If not provided, the [default field resolver is used](data-fetching.md#default-field-resolver).                                                                                                                                                                                                                                                                                                                                                                                                                                                                             |
 | validationRules      | `array` or `callable`                                                       | A set of rules for query validation step. The default value is all available rules. The empty array would allow skipping query validation (may be convenient for persisted queries which are validated before persisting and assumed valid during execution).<br><br>Pass `callable` to return different validation rules for different queries (e.g. empty array for persisted query and a full list of rules for regular queries). When passed, it is expected to have the following signature: <br><br> **function ([OperationParams](class-reference.md#graphqlserveroperationparams) $params, DocumentNode $node, $operationType): array** |
-| queryBatching        | `bool`                                                                      | Flag indicating whether this server supports query batching ([apollo-style](https://www.apollographql.com/blog/apollo-client/performance/query-batching/)).<br><br> Defaults to **false**                                                                                                                                                                                                                                                                                                                                                                                                                                                       |
-| debugFlag            | `int`                                                                       | Debug flags. See [docs on error debugging](error-handling.md#debugging-tools) (flag values are the same).                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       |
-| persistedQueryLoader | `callable`                                                                  | A function which is called to fetch actual query when server encounters a **queryId** without a **query**.<br><br> The server does not implement persistence part (which you will have to build on your own), but it allows you to execute queries which were persisted previously.<br><br> Expected function signature:<br> **function ($queryId, [OperationParams](class-reference.md#graphqlserveroperationparams) $params)** <br><br>Function is expected to return query **string** or parsed **DocumentNode** <br><br> [Read more about persisted queries](https://www.apollographql.com/blog/apollo-client/persisted-graphql-queries).   |
-| errorFormatter       | `callable`                                                                  | Custom error formatter. See [error handling docs](error-handling.md#custom-error-handling-and-formatting).                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      |
-| errorsHandler        | `callable`                                                                  | Custom errors handler. See [error handling docs](error-handling.md#custom-error-handling-and-formatting).                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       |
-| promiseAdapter       | [`PromiseAdapter`](class-reference.md#graphqlexecutorpromisepromiseadapter) | Required for [Async PHP](data-fetching.md#async-php) only.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      |
+| queryBatching        | `bool`                                                                      | Flag indicating whether this server supports query batching ([apollo-style](https://www.apollographql.com/blog/apollo-client/performance/query-batching/)).<br><br> Defaults to **false**                                                                                                                                                                                                                                                                                                                                                                                                                                                      |
+| debugFlag            | `int`                                                                       | Debug flags. See [docs on error debugging](error-handling.md#debugging-tools) (flag values are the same).                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      |
+| persistedQueryLoader | `callable`                                                                  | A function which is called to fetch actual query when server encounters a **queryId**.<br><br> The server does not implement persistence part (which you will have to build on your own), but it allows you to execute queries which were persisted previously.<br><br> Expected function signature:<br> **function ($queryId, [OperationParams](class-reference.md#graphqlserveroperationparams) $params)** <br><br>Function is expected to return query **string** or parsed **DocumentNode** <br><br> [Read more about persisted queries](https://www.apollographql.com/blog/apollo-client/persisted-graphql-queries).   |
+| errorFormatter       | `callable`                                                                  | Custom error formatter. See [error handling docs](error-handling.md#custom-error-handling-and-formatting).                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     |
+| errorsHandler        | `callable`                                                                  | Custom errors handler. See [error handling docs](error-handling.md#custom-error-handling-and-formatting).                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      |
+| promiseAdapter       | [`PromiseAdapter`](class-reference.md#graphqlexecutorpromisepromiseadapter) | Required for [Async PHP](data-fetching.md#async-php) only.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     |
 
 #### Using config class
 
diff --git a/src/Server/Helper.php b/src/Server/Helper.php
index f56a4ef7..21742877 100644
--- a/src/Server/Helper.php
+++ b/src/Server/Helper.php
@@ -255,7 +255,7 @@ class Helper
                 );
             }
 
-            $doc = $op->queryId !== null && $op->query === null
+            $doc = $op->queryId !== null
                 ? $this->loadPersistedQuery($config, $op)
                 : $op->query;
 


Test patch:
diff --git a/tests/Server/QueryExecutionTest.php b/tests/Server/QueryExecutionTest.php
index c6deb43b..3130ede3 100644
--- a/tests/Server/QueryExecutionTest.php
+++ b/tests/Server/QueryExecutionTest.php
@@ -455,15 +455,22 @@ final class QueryExecutionTest extends ServerTestCase
         self::assertSame($expected, $result->toArray());
     }
 
-    public function testExecutesQueryWhenQueryAndQueryIdArePassed(): void
+    public function testLoadsPersistedQueryWhenQueryAndQueryIdArePassed(): void
     {
         $query = /** @lang GraphQL */ '{ f1 }';
 
         $expected = [
-            'data' => ['f1' => 'f1'],
+            'errors' => [
+                [
+                    'message' => 'Cannot query field "invalid" on type "Query".',
+                    'locations' => [['line' => 1, 'column' => 3]],
+                ],
+            ],
         ];
-        $this->config->setPersistedQueryLoader(static function (): array {
-            throw new \Exception('Should not be called since a query is also passed');
+        $this->config->setPersistedQueryLoader(static function (string $queryId, OperationParams $params) use ($query): string {
+            self::assertSame($query, $params->query);
+
+            return /** @lang GraphQL */ '{ invalid }';
         });
 
         $this->assertQueryResultEquals($expected, $query, [], 'some-id');