Implement support for interfaces implementing interfaces #740
Conversation
|
Tests are currently blocked by #741 |
Kingdutch
force-pushed
the
master
branch
5 times, most recently
from
1e5bb57
to
8060642
Compare
Kingdutch
force-pushed
the
master
branch
3 times, most recently
from
c85a619
to
4eeb883
Compare
|
This library sticks pretty close to the reference implementation. For the tests, you should be able to pretty much copy graphql/graphql-js#2084 |
|
I've added the tests from the linked PR :D Let's see if this works in one go or I made a booboo somewhere (or it reveals an implementation error) ^^ |
Kingdutch
force-pushed
the
master
branch
7 times, most recently
from
9f5a420
to
6360356
Compare
|
Wow, that process was a lot rougher than I expected. Who would've guessed it isn't easy to reliably apply a JavaScript diff to a PHP project ^^ Nearly there though(?) |
Kingdutch
force-pushed
the
master
branch
2 times, most recently
from
e58fbc6
to
fde8176
Compare
This ports the JavaScript tests for `RFC: Allow interfaces to implement other interfaces` to PHP. This should ensure that there is sufficient test coverage for the changes made to support interfaces implementing interfaces. Tests taken from https://github.com/graphql/graphql-js/pull/2084/files including any typoes in test description strings to aid in comparison.
This is part of the update to allow interfaces to implement interfaces. A single extend statement to add an implementation of an interface without field declarations is valid. This was caught by tests and brings in a change from graphql/graphql-js#2084
Part of the work done to implement interfaces implementing interfaces. This was caught by test and improves on the previously done changes for the SchemaValidationContext by implementing `validateTypeImplementsAncestors` which was missing.
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
|
@Kingdutch mind if I modify the PR to clarify type signatures? That helps me understand the changes better. |
|
@spawnia Go for it! That's why "allow modifications by maintainers" is enabled :) |
This solves some issues where types couldn't be reconciled because they implemented multiple interfaces. The PR for this in the GraphQL PHP project is now far enough along (all tests passing) that this should be available in an update for us in time. See: webonyx/graphql-php/pull/740
# Conflicts: # src/Utils/SchemaPrinter.php
|
@spawnia It looks like your last changes introduced a coding standards error, any idea why? |
|
Cool! Then this is all green again and ready to be committed :D |
This solves some issues where types couldn't be reconciled because they implemented multiple interfaces. The PR for this in the GraphQL PHP project is now far enough along (all tests passing) that this should be available in an update for us in time. See: webonyx/graphql-php/pull/740
The GraphQL PHP library has a PR ready to implement support for interfaces implementing other interfaces, which is supported by the GraphQL spec. This is relied upon by some of the types in Open Social. We add the changes as a patch to the library so that we can use the version that's shipped with drupal/graphql. See webonyx/graphql-php#740
There was a problem hiding this comment.
That's awesome. Such a huge work! Thank you 👍
There is one small breaking change in BreakingChangesFinder 😄 (see my comment below) It makes me hesitant about publishing it as a minor. What if we revert constant names for 14.x?
This way we'll be able to publish it as minor - then we can rename them in 15.0 and document properly in the migration guide.
So my recommendation would be:
- Rename those constraints back in this PR - then we can merge it and publish as a minor
- Open a separate PR with new names for those constants and a note in the UPGRADE.md. We will merge this PR in 15.0.0
Removing a public constant is a breaking change and can not be implemented in a minor version. Instead the internal value is changed to ensure that existing code keeps working with the new interface implementation logic.
|
@vladar will you add the changelog or should I do it? |
This solves some issues where types couldn't be reconciled because they implemented multiple interfaces. The PR for this in the GraphQL PHP project is now far enough along (all tests passing) that this should be available in an update for us in time. See: webonyx/graphql-php/pull/740
What's new
This adds the option to have interfaces implement other interfaces. I’ve mostly looked at the interface implementation of normal
type. A lot of the work in the parser will continue to work similarly because the specification forces objects to implement all interfaces that a parent interface may implement.Tests
Tests have been implemented according to https://github.com/graphql/graphql-js/pull/2084/files
Backwards Compatibility
There was some discussion in #728 about whether this should be postponed to 15.0 or could be included included in 14.x. From what I can tell so far, all code changes are either backwards compatible, or internal to the library (private in a class). Additionally, the changes don't make any schema that's currently valid, invalid. It does allow schema's that were previously invalid to be valid, but that's not a breaking change :)
Cheers!
I want to commend the previous contributors to this project on the comments littered throughout the codebase. It has helped me using the library in extending projects but also made it easy to understand what is going on while working in the project. I think this project could be a great way for people to learn about ASTs and parsers :D
Closes #728