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

Create field args mapper and cache args resolution #1587

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

Warxcell
Copy link
Contributor

@Warxcell Warxcell commented Jul 21, 2024

Small enhancement/performance optimization + new feature.

We already have input parse value which turns array into something user defined, but same thing for field args' is missing.

And because this could be something expensive (for example - user might validate if args are within defined boundaries) - it doesn't make sense to call that mapper (or even the code that collects the args) more than once for same input. (For example in sub fields of iterable - args won't change in between each iterable).

Did brief test on performance optimization - it was about 15ms on ~800 calls - not something big, but I believe its good to have it - it will be beneficial even to users that doesn't use that args mapper.

src/Executor/ReferenceExecutor.php Outdated Show resolved Hide resolved
src/Executor/ReferenceExecutor.php Outdated Show resolved Hide resolved
src/Type/Definition/FieldDefinition.php Outdated Show resolved Hide resolved
tests/Executor/ExecutorTest.php Outdated Show resolved Hide resolved
tests/Executor/ResolveTest.php Show resolved Hide resolved
@spawnia spawnia changed the title Create field args mapper && cache args resolution Create field args mapper and cache args resolution Jul 22, 2024
@Warxcell
Copy link
Contributor Author

Any idea why phpstan could be falling? I can't run it locally to test, because for some reason it requires php8.2 and I have php8.3 locally.

@spawnia
Copy link
Collaborator

spawnia commented Jul 22, 2024

Any idea why phpstan could be falling? I can't run it locally to test, because for some reason it requires php8.2 and I have php8.3 locally.

Can you not see the output in https://github.com/webonyx/graphql-php/actions/runs/10038021756/job/27739070572?pr=1587?

Error: Method GraphQL\Tests\Executor\ResolveTest::buildSchema() has parameter $testField with no value type specified in iterable type array<string, mixed>.
Error: PHPDoc tag @param for parameter $testField contains unresolvable type.

GitHub also inlines the errors for me:

image

@Warxcell
Copy link
Contributor Author

Warxcell commented Jul 22, 2024

Any idea why phpstan could be falling? I can't run it locally to test, because for some reason it requires php8.2 and I have php8.3 locally.

Can you not see the output in https://github.com/webonyx/graphql-php/actions/runs/10038021756/job/27739070572?pr=1587?

Error: Method GraphQL\Tests\Executor\ResolveTest::buildSchema() has parameter $testField with no value type specified in iterable type array<string, mixed>.
Error: PHPDoc tag @param for parameter $testField contains unresolvable type.

GitHub also inlines the errors for me:

image

I see it, just don't understand it.
Type of param is specified, why it's say it isn't?

And why same code passes check with php7.4 & 8.0 but fails with 8.1, 8.2 & 8.3?

@Warxcell
Copy link
Contributor Author

Locally:

 php -dmemory_limit=-1 ./vendor/bin/phpstan
Note: Using configuration file /home/bozhidar_hristov/projects/graphql-php/phpstan.neon.dist.
 414/414 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                                                        
 [OK] No errors                                                                                                         
                                                                                                                        

🤷

@Warxcell Warxcell requested a review from spawnia July 23, 2024 10:19
@Warxcell
Copy link
Contributor Author

Sorry to bother you, can we merge this? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants