8000 Add webonyx/graphql-php version 0.13 compatibility by antograssiot · Pull Request #2350 · api-platform/core · GitHub
[go: up one dir, main page]
Skip to content

Add webonyx/graphql-php version 0.13 compatibility #2350

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 1, 2018
Merged

Add webonyx/graphql-php version 0.13 compatibility #2350

merged 2 commits into from
Dec 1, 2018

Conversation

antograssiot
Copy link
Contributor
@antograssiot antograssiot commented Nov 27, 2018 •
edited
Loading

waiting for symfony/symfony#29350

8000
Q A
Bug fix? kind of
New feature? no
BC breaks?no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Allow the test suite to run normaly.

@antograssiot antograssiot changed the base branch from master to 2.3 November 27, 2018 20:15
@antograssiot antograssiot mentioned this pull request Nov 27, 2018
Merged
@antograssiot antograssiot force-pushed the sf-bc-break branch 3 times, most recently from f7fb431 to d9d0ba5 Compare November 27, 2018 22:32
@antograssiot
Copy link
Contributor Author
antograssiot commented Nov 27, 2018 •
edited
Loading

I'll have a look to the other PHPStan issue later if we are going to support several version of webonyx/graphql-php as I started.
done

@antograssiot antograssiot force-pushed the sf-bc-break branch 2 times, most recently from 4a35f76 to 7a684a8 Compare November 28, 2018 04:51
@antograssiot antograssiot changed the title Avoid Symfony BC break Avoid Symfony BC break and add webonyx/graphql-php version 0.13 compatibility Nov 28, 2018
@alanpoulain
Copy link
Member

Wouldn't it be better to force webonyx/graphql-php to be >=0.13?

@antograssiot
Copy link
Contributor Author

that's indeed an option, we already support php7.1+ so it should not be an issue for people to upgrade.
I wasn't sure of what was the policy regarding previous upgrade of this library @alanpoulain.

My only concern is that they change the default errors formatting, and people might be relying on this in their app, they would not have way to stick to the old version.

@alanpoulain
Copy link
Member

The previous update was done by forcing the next version.
It's because the GraphQL part of API Platform is an experimental feature. It allows us to do some breaking changes.
In this case, I am not sure about what to do. The code will be less readable with the method_exists but it's mainly in the tests.
WDYT @api-platform/core-team?

@bendavies
Copy link
Contributor

ah that's the issue i raised:
symfony/symfony#29340

what problem is that causing in api-platform?

@dunglas
Copy link
Member
dunglas commented Nov 28, 2018

Regarding GraphQL, I'm all for bumping the version. It is still marked as @experimental anyway, and it'll stay like that until webonyx/graphql releases a 1.0 version.

@antograssiot
Copy link
Contributor Author

ok I'll update tomorrow

@antograssiot
Copy link
Contributor Author
antograssiot commented Nov 29, 2018 •
edited
Loading

@bendavies yes that's the issue you raisd. I broke this test (children was an empty array) it also broke nelmio/alice fixtures loading in some testsuite for example (that's how I discovered it in the first place)

@alanpoulain I've bump webonyx/graphql-php as discussed

< 8000 /svg>

alanpoulain
alanpoulain reviewed Nov 29, 2018
View reviewed changes
alanpoulain
alanpoulain reviewed Nov 29, 2018
View reviewed changes
alanpoulain
alanpoulain reviewed Nov 29, 2018
View reviewed changes
@alanpoulain
Copy link
Member

@antograssiot could you add a Behat test for the debugging? I think it should be possible if you send a bad query for example.

@antograssiot
Copy link
Contributor Author

yeah, I just ran out of time this morning @alanpoulain

Taluu
Taluu reviewed Nov 30, 2018
View reviewed changes
@antograssiot antograssiot changed the title Avoid Symfony BC break and add webonyx/graphql-php version 0.13 compatibility Add webonyx/graphql-php version 0.13 compatibility Nov 30, 2018
@antograssiot antograssiot force-pushed the sf-bc-break branch 2 times, most recently from 146551b to de56ae3 Compare November 30, 2018 22:30
@antograssiot
Copy link
Contributor Author

Tests added @alanpoulain
I've also removed the version constraint update for Symfony and update the PHPStan rule to follow the Symfony 4.2 release
It should be good this time

i did cleanup some old @dropSchema annotation that were still th 8000 ere at the same time

alanpoulain
alanpoulain approved these changes Dec 1, 2018
View reviewed changes
Copy link
Member
@alanpoulain alanpoulain left a comment

Choose a reason for hiding this comment

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

LGTM Great job!

@alanpoulain alanpoulain merged commit 07980c1 into api-platform:2.3 Dec 1, 2018
@alanpoulain
Copy link
Member

Thank you @antograssiot 🙂

@antograssiot antograssiot deleted the sf-bc-break branch December 1, 2018 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Taluu Taluu Taluu left review comments

@dunglas dunglas dunglas approved these changes

@alanpoulain alanpoulain alanpoulain approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@antograssiot @alanpoulain @bendavies @dunglas @Taluu
0