-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
Fix class based schemas within routes #724
Conversation
We cannot use the Facade in the routes: this would fully have and unconditionally boot GraphQL which is not acceptable for the routes. Seems the solution needs a stateless variant of To goal would be either to:
At least, these are my first quick thoughts on this. |
No worries. I made a draft so I can run the test suite. I'll fix it up. Thanks for the tips. |
This is ready for review whenever you are. (In case you didn't get the notification - not hassling you) |
ping is good! problem is, I never know when "it's done" and I don't get notification when the draft/non-draft state changes. And I guess you can't just select a user from the reviewer request. #JustThingsGithub I've taken note and will check it out once I've time for it! |
I re-requested a review, just to see if you get a notification. 🤷 yes GitHub things. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so I tried to run this in a private project and basically GraphQL totally broke; got about 5k tests and none of them work anymore 😏
Some issue with type resolution:
array (
'errors' =>
array (
0 =>
array (
'status' => '500',
'code' => '0',
'title' => 'Rebing\\GraphQL\\Exception\\TypeNotFound: Type <SomeType> not found.
Check that the config array key for the type matches the name attribute in the type\'s class.
It is required when \'lazyload_types\' is enabled',
'meta' =>
array (
'stacktrace' => '#0 /project/vendor/rebing/graphql-laravel/src/GraphQL.php(193): Rebing\\GraphQL\\GraphQL->getType(\'SomeType\', false)
#1 /project/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php(261): Rebing\\GraphQL\\GraphQL->type(\'SomeType\')
#2 /project/app/GraphQL/Queries/SomeQuery.php(34): Illuminate\\Support\\Facades\\Facade::__callStatic(\'type\', Array)
I haven't yet dug into it.
Co-authored-by: Markus Podar <markus@fischer.name>
Well that's not great. 🤔 I'll try to work that out. |
# Conflicts: # CHANGELOG.md
@jasonvarga it was a false alarm I think 😬 This PR wasn't up2date with master and when I tried it in a project, it was already relying on other improvements in master which are not yet in here and thus broke I think. I therefore resolved the conflict and merged master, will try again now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
Especially that it does not have any additional penalty unless you use the class based schema looks well done to me, thank you!
Thank you as well! |
return array_filter(array_map(function ($schema) { | ||
try { | ||
return static::getNormalizedSchemaConfiguration($schema); | ||
} catch (SchemaNotFound $e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonvarga do you remember why you added this here?
I was playing around with some refactoring in other parts when I saw this and I wondered: if a schema is broken, aka not found, why would I silence the schema here?
Aka: a broken schema throws an exception for a purpose, it should crash the app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. 😬
In 6.2.0, if you had an invalid class (or just a string, really, since class names weren't supported yet) it would not throw an exception. I think I just made it continue to not throw an exception to keep the behavior consistent and not a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I decided to change it (#766) because somehow it didn't make sense to have an invalid configuration being silently skipped (as this is used for generated the routes, you're suddenly left in the dark why a certain schema simply "isn't there").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No arguments here!
Summary
In #706 I added support for you to be able to generate a schema config by supplying a class name.
That worked when querying, but I've just noticed it doesn't work when generating the routes.
i.e.
middleware
andmethod
wouldn't be respected.In
routes.php
, the routes are created by simply reading the config and looping over the schemas. If a class name is used, it needed to be converted to the array.Type of change:
Checklist:
composer fix-style