-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Error when store a big data to cache #1268
Comments
Can be reproduced when the schema definition reaches a certain size. Indeed
Using php 7.4. |
Hi , I'm also having the same issue , and yes, serialize functions reach their limit because of the complexity of the |
I also got this when worked on a project. I reverted the schema to its previous state, so it was big (1100+ lines), but it was still serializable. Then I did some measures comparing the schema-parse time versus the cache-get time.
And it looks like there is almost no difference. At least in my case (PHP 7.4, SQLite). With xdebug isn't that goodxdebug enabled, its port is not listened:
xdebug enabled, phpstorm is listening to its port:
In the end I used a workaround to disable the schema caching by adding But after all, does it make sense to cache the parsed schema if this brings almost zero benefit? |
Perhaps we could just cache the raw text schema so it doesn't have to be fetched from files each time and parse that for each request? |
The problem is that \GraphQL\Language\Token has self-references and the serializer can't deal with the endless tree that creates: The documentation for seralize() specfically states that some things just aren't serlializable, so I propose we don't try to do it here. |
Issue filed with webonyx/graphql: |
TLDR: Can we use My journey here 🙃 When I first met Then I got another project with a bigger schema. I tried Now I'm on a project with an even bigger schema. The parsed AST can't be serialized, the However, the GraphQL performance dropped. GraphQL calls went almost two times slower. Finally, I tried I was expecting that this option will do some harm, like error messages will have no location information. Yet the error messages look just as before. So probably it's safe to use |
Maybe we could bind that to |
We have tests for errors graphql/tests/src/Kernel/Framework/LoggerTest.php Lines 114 to 132 in 122e29c
They still work with noLocation .
|
Merged the PR, thanks a lot! I will push a small docs follow-up to document the why in code, as this noLocation option is not obvious. |
sorry, I just want try to report this issue.
The text was updated successfully, but these errors were encountered: