thecodingmachine / graphqlite

Use PHP Attributes/Annotations to declare your GraphQL API
https://graphqlite.thecodingmachine.io
MIT License
554 stars 95 forks source link

Use ComposerFinder's useAutoloading method #667

Closed oprypkhantc closed 4 months ago

oprypkhantc commented 4 months ago

Closes #659

This implementation is much simpler: now that we use kcs/class-finder and that it has autoloading enabled by default, the only thing left to do is provide an option to disable it if you need the "legacy" behaviour for whatever reason.

I've tested the new autoloading flow in our project and it's behaving exactly as expected.

oojacoboo commented 4 months ago

@oprypkhantc Can't you just set your own customized Finder instance on the SchemaFactory, instead of us modifying it to support this edge case?

$finder = (new ComposerFinder)->useAutoloading(false);
$schemaFactory->setFinder($finder);
oprypkhantc commented 4 months ago

@oojacoboo This PR is mostly for backwards compatibility. Let me explain:

Up until now, graphqlite used include_once to load classes - both before the migration to kcs/class-finder and after. The most recent version of kcs/class-finder at the moment is 0.4.0, which still uses include_once by default, effectively meaning useAutoloading(false).

Now the author of the library pushed a commit to master changing the default to use autoloading instead, meaning once we upgrade to 0.5.0, the behaviour will change for all users of graphqlite. It also makes some tests in graphqlite fail.

With this PR, I've upgraded to dev-master (0.5.0 as soon as the release is out) and added an autoloading opt-out option for those who need to use the legacy include_once behaviour for whatever reason. I can remove this option of course and I'll gladly do so :)

We'll still need this PR to fix the tests and add a kcs/class-finder ^0.5.0 constraint though :)

oojacoboo commented 4 months ago

@oprypkhantc yep - got it.

I think we remove this option in GraphQLite and update documentation on how to disable using something like the example code I provided above.

oprypkhantc commented 4 months ago

@oojacoboo Done :) Added a small section in the docs about autoloading. Also I'm not sure why docs generation fails on CI - it builds locally no problem 👀

oojacoboo commented 4 months ago

Merged. Sometimes the test runners just crap out and need to be re-run.