-
-
Notifications
You must be signed in to change notification settings - Fork 438
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
Refactor builder types #2389
base: master
Are you sure you want to change the base?
Refactor builder types #2389
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.
Good change, I am all for making this switch in the next major version.
I did some regex replacements for the |
# Conflicts: # src/Pagination/PaginationArgs.php # src/Schema/Directives/AggregateDirective.php
I cleaned up a couple of remaining places where the interface could be used and added another change on top. It is somewhat unrelated but touches the same parts of the code and is also breaking, so I figured we might as well save ourselves the trouble of resolving merge conflicts and add it in one go. |
# Conflicts: # CHANGELOG.md # src/WhereConditions/WhereConditionsHandler.php
i think this pr is fine, i don't understand why my change would make codecov care all of a sudden. |
No worries, we can just ignore that.
Merge to |
In laravel 9, a new interface
Illuminate\Contracts\Database\Query\Builder
was added to improve the readability of typehints likeIlluminate\Database\Query\Builder|Illuminate\Database\Eloquent\Builder|Illuminate\Database\Eloquent\Relations\Relation
.This pr is just some refactoring to use that interface and I also found some duplicate code which i moved to a trait.
(Breaking) Changes
When building custom directives, people now need to use the new Builder interface to typehint. So this would be something for a v7.