Fix apq with page caches #1317
Fix apq with page caches #1317
Conversation
| /** | ||
| * The base class for all functional GraphQL tests. | ||
| */ | ||
| abstract class GraphQLFunctionalTestBase extends BrowserTestBase { |
There was a problem hiding this comment.
we have a HttpRequestTrait that we use in the kernel tests, can we use that one to test the request/response workflow? Maybe we need to enhance it a bit?
Then we would not need to invent a new functional test category here.
|
Thanks for the review! I am going to take a look at the requested changes. |
|
Thanks, can you answer all the inline comments from above and click "resolve" if they are done in your opinion? The biggest blocker here is adding the Functional tests, which I would like to avoid if we can. |
|
I have been able to replace the functional tests. But I also had to implement a different approach to solve the dynamic page cache issue. The page cache fix is basically the same as before, but the dynamic page cache fix was not really working, as I found out while trying to write a new test for it... So, I basically turn off dynamic page cache for the graphql requests. I think that is totally fine. Dynamic page cache does not seem to be the right thing for APQ requests anyway. But I think, @pmelab and @IT-Cru might want to test this new approach in their projects again, to make sure it still works for them. |
|
The last commit was done by @alexpott, not by me! He found a way to correctly add the cache context for the dynamic page cache. This looks quite good now I think! |
| $parameters['variables'] = '{"id": "2"}'; | ||
| $content = json_encode(['query' => $titleQuery] + $parameters); | ||
| $request = Request::create($endpoint, 'POST', [], [], [], ['CONTENT_TYPE' => 'application/json'], $content); | ||
| $result = $this->container->get('http_kernel')->handle($request); |
There was a problem hiding this comment.
why do we have to do all this repetitive request building and cannot use $this->query() from HttpRequestTrait?
Opened #1393 as this was also not correct in the previous test case.
| if (is_string($query)) { | ||
| $computedQueryHash = hash('sha256', $query); | ||
| if ($queryHash !== $computedQueryHash) { | ||
| throw new Error('Provided sha does not match query'); |
There was a problem hiding this comment.
I think Error is not correct here, should be UserError, right? We want to show this error to the graphql client that they sent something wrong.
Not the fault of this pull request, was already wrong before.
There was a problem hiding this comment.
Interestingly we are asserting in the tests that the error message is coming out in the response, so it appears to be treated already as user error for some reason ... so we can leave this as is and don't care for now.
|
Thanks a lot! I will merge this now and then simplify the test case a bit to use the |
|
While converting the test cases I got super confused, why it fails on GET requests and shows a cached result. Need to find out why in #1394. |
|
Of course it was my fault, not using the result value from the method call. All good now and will merge the follow-up test improvements soon. |
This is aiming to resolve #1316 and resolve #1315.
Why one PR for both issues?
The reason why I want to fix both in one PR is because I started with writing tests for it. And both tests need to be BrowserTestBase tests, because otherwise we would not have requests, that create page cache (at least I did not find a way to write a kernel test).
Since no BrowserTestBase tests are in the module yet, 99% of the PRs would be identical, or they would have to be stacked.
Structure of PR
Most of the PR is just adding the tests, and fixing problems, that are caused by this - changing testing.yml, updating guzzle, fixing new phpstan issues caused by that, and the like.
If you just want to take a look at the actual fixes, then two commits are of interest:
Page Cache Fix
d5e9485
Dynamic Page Cache fix
2e6e282