-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
Relax PaginationType getPaginationFields typehint #1132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change is ok!
Also relaxing should not be a breaking change, so we would not need to do a major version bump, WDYT?
Can you please add a dedicated test, i.e. one which covers what you needed it for (support for interface type)?
(I can take care of the phpstan once all is done here)
Nah I don't think it needs to be breaking.
Can do 👍 Thanks! |
I've added/modified tests where the existing pagination test was. There's an assertion on the generated sql that doesn't currently pass because of an unrelated issue, so I've left it in there but commented out: The sql should be I think it's out of scope of this PR to fix it. Anyway, the tests in this class would fail without the typehint change in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work, thank you!
Will roll a release right after merge.
Thank you! |
Summary
When upgrading a project from 8.0 to 9.0, I began to see errors around pagination:
We have queries where the
type
is a paginator with interfaces:This has worked fine until v9 (specifically #1033) since the typehint of
PaginationType::getPaginationFields()
was changed toObjectType
.This PR changes the typehint from an
ObjectType
definition to the parentType
definition, which covers bothObjectType
andInterfaceType
.I'm not sure if there's a specific reason
ObjectType
was used, but this fixes our issue and all of this package's tests continue to pass.Also, I'm sorry but I don't understand what's up with the phpstan stuff. 😊
Type of change:
Checklist:
composer fix-style