Skip to content

Fix callable type mapping to be Closure mapping #753

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

Merged

Conversation

oprypkhantc
Copy link
Contributor

@oprypkhantc oprypkhantc commented Jul 31, 2025

Quite accidentally I discovered a bug with my implementation: if a value that is also a callable is returned from a field, it will get wrapped in a Deferred wrapper and be called as a callable, instead of simply being returned as a value.

E.g.:

function test() {
    throw new RuntimeException('This is called!');
}

#[Type]
class Test {
    #[Field]
    public string $title = 'test';
}

class TestController {
    #[Query]
    public function test(): Test {
        return new Test();
    }
}

If you then run the test { title } query, instead of simply returning the {"title": "test"}, it would call the test function - because is_callable('test') returns true.

Potential fixes

This could be solved two ways:

  1. remove support for callable, only leave support for actual Closure
  2. keep support for callable, but make it so it wraps values in Deferred based on whether the type was specified as callable, not if the value itself is callable

This PR implements option #1. Option #2 would require some architectural changes to how GraphQLite works, way outside the scope of this feature :)

@oprypkhantc
Copy link
Contributor Author

@oojacoboo It's likely that the issue you described in #739 was due to this. It would be hard to catch otherwise, because it's caused by specific values returned from fields

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.83%. Comparing base (53f9d49) to head (d0d5c9a).
⚠️ Report is 118 commits behind head on master.

Files with missing lines Patch % Lines
src/Mappers/Root/ClosureTypeMapper.php 92.85% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #753      +/-   ##
============================================
- Coverage     95.72%   94.83%   -0.90%     
- Complexity     1773     1845      +72     
============================================
  Files           154      175      +21     
  Lines          4586     5013     +427     
============================================
+ Hits           4390     4754     +364     
- Misses          196      259      +63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@oojacoboo
Copy link
Collaborator

oojacoboo commented Aug 1, 2025

This is a good safe option - thanks! And yes, this was the issue with #739

@oojacoboo oojacoboo merged commit e33f587 into thecodingmachine:master Aug 1, 2025
19 of 20 checks passed
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.

3 participants