Skip to content

Conversation

@oprypkhantc
Copy link
Contributor

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.

@oprypkhantc oprypkhantc marked this pull request as draft March 22, 2024 00:34
@oojacoboo
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

@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 👀

@oprypkhantc oprypkhantc marked this pull request as ready for review March 26, 2024 15:02
@oojacoboo oojacoboo merged commit 23d6c9d into thecodingmachine:master Mar 27, 2024
@oojacoboo
Copy link
Collaborator

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

@oprypkhantc oprypkhantc deleted the autoload2 branch March 27, 2024 10:20
faizanakram99 added a commit to faizanakram99/graphqlite that referenced this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GraphQLite attempts to load non-class files, resulting in weird errors

2 participants