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

❗BREAKING CHANGE❗️Ensure compatibility with graphql-php v15 #953

Merged
merged 32 commits into from
Mar 5, 2023

Conversation

mfn
Copy link
Collaborator

@mfn mfn commented Oct 15, 2022

Summary

See webonyx/graphql-php#1231

❗ Merging this will require a major version bump due to all around incompatible changes forced by graphql-php ❗

TODO

  • Figure out how to fix or workaround the disabled test \Rebing\GraphQL\Tests\Unit\ConfigTest::testSecurity
  • Wait for official v15 release and remove hack commit "graphql-php-v15: workaround to allow testing with graphql-php v15"
  • Then make sure integration tests are green
    They current aren't due to the composer.json hack

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist:

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

@mfn mfn self-assigned this Oct 15, 2022
@mfn mfn force-pushed the mp-graphql-php-v15 branch 2 times, most recently from ec0da08 to bf2668a Compare October 17, 2022 19:48
@mfn mfn changed the title Ensure compatibility with graphql-php 15 Ensure compatibility with graphql-php v15 Oct 17, 2022
composer.json Outdated
@@ -38,9 +38,9 @@
"ext-json": "*",
"illuminate/contracts": "^6.0|^8.0|^9.0",
"illuminate/support": "^6.0|^8.0|^9.0",
"laragraph/utils": "^1",
"laragraph/utils": "1.4.1",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"laragraph/utils": "1.4.1",
"laragraph/utils": "dev-graphql-php-15",

Should be the easier temporary fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh my… I never connected the dots, that what follows dev-… can be any qualifier, even a branch name 🤦🏼

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice, thanks, everything is green!

mfn added 15 commits October 18, 2022 14:13
A later commit adds support back for our own error classes.
Until we figure out what the replacement for the getter is
…ReadOnly` have been dropped in favour of the public properties
@leonardocustodio
Copy link
Contributor

We are having this issue when running tests on PHP 8.2, I guess because rebing is still using the older webonyx lib?

PHP Deprecated: Using ${var} in strings is deprecated, use {$var} instead in /vendor/webonyx/graphql-php/src/Validator/Rules/ValuesOfCorrectType.php on line 182

@mfn
Copy link
Collaborator Author

mfn commented Jan 27, 2023

Yes, this sounds about right; and this PR would fix this.

@websitevirtuoso
Copy link
Contributor

v15 has been released
https://github.com/webonyx/graphql-php/releases
Can we check this PR one more time please?
Let me know If I can assist in this

@sforward
Copy link
Contributor

sforward commented Mar 1, 2023

I have been testing these changes against our tests. I'm running into an exception when printing the generated schema via the upstream GraphQL\Utils\SchemaPrinter::doPrint. I've made a test case and a solution, but not sure if that's the best solution:

0fc11f6

@mfn
Copy link
Collaborator Author

mfn commented Mar 1, 2023

Cool find @sforward ! Can you make a PR against master with just the test?

Assuming it would just work, next time I rebase this PR I'll then fix it considering your suggestion in the commit.

WDYT?

@sforward
Copy link
Contributor

sforward commented Mar 1, 2023

Sure, I've created a MR with the test!

# Conflicts:
#	.github/workflows/tests.yml
#	CHANGELOG.md
#	composer.json
#	phpstan-baseline.neon
#	phpstan.neon.dist
#	tests/Unit/GraphQLTest.php
@mfn
Copy link
Collaborator Author

mfn commented Mar 5, 2023

I have been testing these changes against our tests. I'm running into an exception when printing the generated schema via the upstream GraphQL\Utils\SchemaPrinter::doPrint. I've made a test case and a solution, but not sure if that's the best solution:

This seems to be related to the recent change in webonyx/graphql-php#1303 . If I revert these changes in SchemaPrinter.php, the test passes.

Not yet sure what the expectation is here.

@mfn
Copy link
Collaborator Author

mfn commented Mar 5, 2023

@sforward I came up with an alternate fix: it seems the change in graphql-php now expects the lazy type loader to also answer for the top level Mutation (etc.) types, which no one ever realized because no one needed it.

I've added them in the latest commit but also opened webonyx/graphql-php#1336 to see if that's actually intentional.

@mfn mfn merged commit af54a0f into master Mar 5, 2023
@mfn mfn deleted the mp-graphql-php-v15 branch June 26, 2023 18:10
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.

Using getRelations of SelectFields involves only selecting the requested fields for relationships
6 participants