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

Refactor builder types #2389

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Refactor builder types #2389

wants to merge 21 commits into from

Conversation

thekonz
Copy link
Collaborator

@thekonz thekonz commented Apr 28, 2023

In laravel 9, a new interface Illuminate\Contracts\Database\Query\Builder was added to improve the readability of typehints like Illuminate\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.

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

(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.

@thekonz thekonz marked this pull request as ready for review April 28, 2023 14:47
@spawnia spawnia added 7.x Related to the 7.x release series enhancement A feature or improvement labels Apr 30, 2023
Copy link
Collaborator

@spawnia spawnia left a 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.

src/Schema/Directives/Traits/HasBuilderArgument.php Outdated Show resolved Hide resolved
src/Schema/Directives/Traits/HasBuilderArgument.php Outdated Show resolved Hide resolved
src/Schema/Directives/Traits/HasBuilderArgument.php Outdated Show resolved Hide resolved
src/Schema/Directives/Traits/HasBuilderArgument.php Outdated Show resolved Hide resolved
src/Schema/Directives/Traits/HasBuilderArgument.php Outdated Show resolved Hide resolved
src/Schema/Directives/Traits/RelationDirectiveHelpers.php Outdated Show resolved Hide resolved
src/Support/Contracts/ArgBuilderDirective.php Outdated Show resolved Hide resolved
@thekonz
Copy link
Collaborator Author

thekonz commented May 2, 2023

I did some regex replacements for the @param docblocks and resolved other review issues.

UPGRADE.md Show resolved Hide resolved
src/Execution/ResolveInfo.php Outdated Show resolved Hide resolved
src/Pagination/PaginationArgs.php Outdated Show resolved Hide resolved
src/Schema/Directives/AllDirective.php Show resolved Hide resolved
src/Support/Contracts/ArgBuilderDirective.php Outdated Show resolved Hide resolved
@spawnia spawnia mentioned this pull request May 3, 2023
@spawnia
Copy link
Collaborator

spawnia commented May 3, 2023

@thekonz I included the parts that clean up existing code in #2390.

@spawnia
Copy link
Collaborator

spawnia commented May 3, 2023

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
@thekonz
Copy link
Collaborator Author

thekonz commented May 25, 2023

i think this pr is fine, i don't understand why my change would make codecov care all of a sudden.
what's your merge strategy? merge v7 changes to master or a separate v7 branch?

@spawnia
Copy link
Collaborator

spawnia commented May 25, 2023

i don't understand why my change would make codecov care all of a sudden.

No worries, we can just ignore that.

what's your merge strategy? merge v7 changes to master or a separate v7 branch?

Merge to master once we decided to make the cut towards v7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.x Related to the 7.x release series enhancement A feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants