New Symfony and GraphQLite versions

[fogrye]

Fixes #202

Wait for thecodingmachine/graphqlite-symfony-validator-bridge#70

[homersimpsons]

Thank you for your work on this!

I do not have access to a computer so I just did a fast review so you get some early feedback.

Do not hesitate to discuss them.

@moufmouf Do not hesitate to step up if you need to add anything. In particular I'm not sure what is part or not part of the public api for breaking changes.

[fogrye]

@homersimpsons thank you for your quick review, as soon as validator bridge will be approved I will make this PR ready

[fogrye]

@homersimpsons I would push this as BC because of BC of graphqlite itself

[cvergne]

@fogrye 👋 Not a significant feedback, but don't forget to update the README by bumping 5 to 6 here :

graphqlite-bundle/README.md

Line 10 in 9fc3c79

Symfony 5 bundle for the thecodingmachine/graphqlite package.

[faizanakram99]

@fogrye @homersimpsons

whats the status here?

i think it makes sense to remove graphiql bundle too in this PR, uses can include and configure it themselves, this bundle is about symfony integration, it doesn't have to include a graphiql explorer especially when it blocks framework integration.

[fogrye]

@faizanakram99 hi, thx for your feedback but I think it's better to keep this PR simple. I have created separate issue to discuss future of graphiql here.

This PR is ready to be reviewed, @homersimpsons could you pls take a look, thx.

[homersimpsons]

Why is this upgraded? Is it just for symfony 7? In this case it may be possible to keep it to 8.1 as it is still supported.

Suggested change

	"php" : ">=8.2",
	"php" : ">=8.1",

[fogrye]

the problem is symfony/validator itself

[homersimpsons]

Thank you for your work on this @fogrye! Just 2 small discussions as I am not 100% familiar with this project and it should be ready to merge.

As there are no external api changes this will certainly be released as a minor version.

We can keep the major update if we remove GraphiQL.

[xlttj]

When do you plan to release this PR? We're using Symfony 7 and evaluating several GraphQL related projects.

[faizanakram99]

@fogrye i am using your fork temporarily, i saw huge performance regression (earlier it was a few ms, now it is more than a minute). I browsed through the changes and there is just one change, i.e. usage of Kcs/ClassFinder.

[faizanakram99]

ok i see that GlobClassExplorer used to cache class list (using either apcu or php files adapter), the new class list isn't cached, that is what is causing regression.

[fogrye]

@fogrye i am using your fork temporarily, i saw huge performance regression (earlier it was a few ms, now it is more than a minute). I browsed through the changes and there is just one change, i.e. usage of Kcs/ClassFinder.

It may happen if you have big dependency tree without classmap cache I believe.

[faizanakram99]

@fogrye i am using your fork temporarily, i saw huge performance regression (earlier it was a few ms, now it is more than a minute). I browsed through the changes and there is just one change, i.e. usage of Kcs/ClassFinder.

It may happen if you have big dependency tree without classmap cache I believe.

We're using psr4 autoloading and always use optimized autoloader, that is not the problem. I first noticed it in phpunit tests, smoke test which would take a few ms would take a minute each.

This is the classmap (it is not empty)

See the difference here

Before (with graphqlite bundle stable version)

After (with graphqlite bundle of this fork)

As you can see, earlier it was a 1 or 2 seconds for each request (see the main request and the ajax requests), now it is at least 45 seconds for each request.

[fogrye]

@faizanakram99 It would be nice if you can profile the code, maybe because new Finder operating with such big array it becomes slow. This issue is more a problem of graphqlite major update and I appreciate if you can create issue there with results of profiling. Thank you.
Edit:
Or maybe it's not and I have to cache something in symfony bundle. I also will take a look.

[faizanakram99]

@faizanakram99 It would be nice if you can profile the code, maybe because new Finder operating with such big array it becomes slow. This issue is more a problem of graphqlite major update and I appreciate if you can create issue there with results of profiling. Thank you. Edit: Or maybe it's not and I have to cache something in symfony bundle. I also will take a look.

Yes I profiled it, it is indeed due to kcs/class-finder component, it is alone taking more than a minute.

You're right, the problem is in main package. Since the usage here is in compiler pass, I think that only runs during container compilation and shouldn't run for each request (not sure though).

[faizanakram99]

ok i used a custom implementation of FinderInterface and added it to SchemaFactory using a compiler pass and now it only takes 2 seconds. I noticed that only one method of FinderInterface is called (find in namespace).

https://gist.github.com/faizanakram99/a7241274a3f923a3d2c78dc20c0a5ca8

I think in symfony apps it makes sense to only look up folders which are registered as root for graphqlite types or resolvers (like done in the gist). A generic implementation based on the example above might need relative path (relative to kernel.project_dir) for graphqlite types and resolvers in addition to namespaces.

Either way looking up all classes in the class map makes little sense to me (especially for a symfony bundle).

[fogrye]

@faizanakram99 great that you found solution for your case, finder was changed to support types from dependencies. To make it work efficiently caching have to be implemented to work efficiently.

[faizanakram99]

@faizanakram99 great that you found solution for your case, finder was changed to support types from dependencies. To make it work efficiently caching have to be implemented to work efficiently.

I think it should be opt in, it shouldn't aggressively scan all classes. I don't think any framework does so, symfony for example requires resource root to be registered for each top level namespace/bundle, something similar should be done here. This is very inefficient.

My workaround fixes the runtime issues but cache clear is still slow because the compiler pass in symfony bundle still uses ComposerFinder.

[fogrye]

I think it should be opt in, it shouldn't aggressively scan all classes.

For sure, unfortunately that is how lib we picked works (and we hadn't a lot of libs to pick from). We have to come up with solution for that. Maybe propose change for kcs/class-finder. I'm sorry that we missed that in first place.

[fogrye]

@faizanakram99 I have created an issue for this alekitto/class-finder#21

[alekitto]

Hi there,
I'm the author of the class-finder lib, I've read of the performance issue you encountered.

I'll try to respond to the linked issue for a more general solution, but here we are in a specific context where a specific solution is required.

In Symfony's context, a full scan should be triggered only when the container is rebuilt. Additionally, only if some specific paths are given, the use of in or paths filter will greatly speed up the find operation. The hack showed up in the gist is not necessary, as the finder is already capable of filtering directories and specific paths.

The CompilerPass solution is probably the best approach (is the one I use in my projects).
This also guarantees that there's no such issue in non-debug environments (mainly prod), while in dev you should encounter a slow down only if the Symfony cache is cleared or one (or more) Resource object triggers a container rebuilding.

We can discuss on how we can optimize the whole process for non-Symfony projects on the lib issue.

I will gladly help to implement a more efficient Symfony-specific solution, but I'm on vacation ATM, so I cannot give a real contribution before the next week.

[jroszkiewicz]

Hey, any progress? We already got symfony 7.1, but graphqlite is locked to 6.4 :(

[homersimpsons]

Hey, any progress? We already got symfony 7.1, but graphqlite is locked to 6.4 :(

I think we should wait for the performance improvements. I am not familiar enough with this codebase to push it forward quickly.

[homersimpsons]

For the record, I tried to push it forward but I came across issues while running tests vendor/bin/simple-phpunit:

1. 'enable_authenticator_manager' property no longer exist in security bundle, I removed it
2. PHP Fatal error: Declaration of Symfony\Component\Cache\Traits\Redis5Proxy::_compress($value) must be compatible with Redis::_compress(string $value): string in /usr/src/app/vendor/symfony/cache/Traits/Redis5Proxy.php on line 64 This looks like this is because the class finder tries to load this class, but I have a Redis6. I guess this is because we are trying to reflect all the classes.

[alekitto]

2. `PHP Fatal error:  Declaration of Symfony\Component\Cache\Traits\Redis5Proxy::_compress($value) must be compatible with Redis::_compress(string $value): string in /usr/src/app/vendor/symfony/cache/Traits/Redis5Proxy.php on line 64` This looks like this is because the class finder tries to load this class, but I have a Redis6. I guess this is because we are trying to reflect all the classes.

This is explained here: alekitto/class-finder#13 (comment)

Could be resolved easily with class-finder 0.5 (supported in master version of graphqlite): just call skipBogonFiles method on the newly created ComposerFinder to skip the files known to cause fatal errors (for the records, you can find the regex matching the files to be skipped here: https://github.com/alekitto/class-finder/blob/4b8d4bbb86d311205e882c1a06d8c93e3a36d1a8/lib/Util/BogonFilesFilter.php)

[jaapio]

Hi, I came across this pr and I was wondering what the status is. I'm willing to spend some time to make the upgrade to symfony 7.x possible but only when I know that nobody solved the issues already.