Skip to content
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 infinite loop #579

Merged
merged 2 commits into from
Feb 1, 2020
Merged

Fix infinite loop #579

merged 2 commits into from
Feb 1, 2020

Conversation

crissi
Copy link
Contributor

@crissi crissi commented Jan 19, 2020

Summary

Fixes #512

Fix the infinite loop as well as sending the correct matching input data to the rule-callback.

If people have been using rule-callback in a nested input object, the input to the object was before always the full input, now the input data matches the level of nesting.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@crissi crissi force-pushed the fix_infinite_loop branch 2 times, most recently from de79c2e to 5c58aee Compare January 21, 2020 15:33
@mfn
Copy link
Collaborator

mfn commented Jan 22, 2020

Ok! That one looks massive…
will take me some time to dig through it; thanks!

@mfn
Copy link
Collaborator

mfn commented Jan 25, 2020

Sorry for the long wait, I've looked into the high level functionality: very cool, totally want to merge this!

Going now through the code for giving detailed feedback.

@crissi
Copy link
Contributor Author

crissi commented Jan 25, 2020 via email

@mfn mfn self-requested a review January 25, 2020 21:18
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any issues with the general implementation!

However I'm not super happy with some aspects of the under-documented types, especially arrays
image

I therefore now updated larastan which depends on a newer phpstan version which is much more strict in this regard. But since generates > 600 errors on master, I added a baseline file; meaning only new code parts will trigger such phpstan errors => like this PR likely will do.
This is kind of an experiment, if it's not working out, I'll remove it.

For that reason, and the in the meantime also updated changelog.md, please rebase with master and add a changelog entry => well deserved :)

src/Support/AliasArguments/AliasArguments.php Outdated Show resolved Hide resolved
src/Support/AliasArguments/AliasArguments.php Outdated Show resolved Hide resolved
tests/Unit/RecursionTest/RecursionTest.php Outdated Show resolved Hide resolved
src/Support/Field.php Outdated Show resolved Hide resolved
src/Support/Rules.php Outdated Show resolved Hide resolved
tests/Unit/MutationTest.php Outdated Show resolved Hide resolved
tests/Unit/MutationTest.php Outdated Show resolved Hide resolved
tests/Unit/MutationTest.php Outdated Show resolved Hide resolved
tests/Unit/MutationTest.php Outdated Show resolved Hide resolved
tests/Unit/MutationTest.php Outdated Show resolved Hide resolved
@crissi crissi requested a review from mfn January 29, 2020 21:17
@crissi
Copy link
Contributor Author

crissi commented Jan 29, 2020

updated

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks (also for the laborious work!)

@mfn mfn merged commit 95b7a0d into rebing:master Feb 1, 2020
@mfn
Copy link
Collaborator

mfn commented Apr 3, 2020

https://github.com/rebing/graphql-laravel/releases/tag/5.0.0 has been released which includes this!

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.

Infinite loop while input object pointing at each other
2 participants