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

Allow Builder-s in Builder::union* $query parameter #1231

Merged
merged 1 commit into from
May 23, 2022

Conversation

fagai
Copy link
Contributor

@fagai fagai commented May 13, 2022

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

related: laravel/framework#42374

The Builder::union method is supports Illuminate\Database\Eloquent\Builder, but an error occurs because there is no description.

Steps to reproduce

$first = User::where('id',8);

$users = User::where('id',2)->union($first)->get();

Error Report

Parameter #1 $query of method Illuminate\Database\Eloquent\Builder<Illuminate\Database\Eloquent\Model>::union() expects Closure|Illuminate\Database\Query\Builder, Illuminate\Database\Eloquent\Builder given.

Description

Illuminate\Database\Eloquent\Builder is not a subtype of Illuminate\Database\Query\Builder.

Illuminate\Database\Eloquent\Builder

use Illuminate\Contracts\Database\Eloquent\Builder as BuilderContract;
class Builder implements BuilderContract
{

Illuminate\Database\Query\Builder

use Illuminate\Contracts\Database\Query\Builder as BuilderContract;
class Builder implements BuilderContract
{

Changes

Added type.

Breaking changes

no

@szepeviktor szepeviktor changed the title add phpdoc types Allow Builder-s in Builder::union* $query parameter May 13, 2022
@szepeviktor
Copy link
Collaborator

Hello @fagai! 👋
Thank you for your contributions.
Builder is a generic class, please see
https://github.com/nunomaduro/larastan/blob/1162e3b3ca2cee6ab77b67bfdf20d598ebf50f20/stubs/QueryBuilder.stub#L139

@fagai
Copy link
Contributor Author

fagai commented May 13, 2022

@szepeviktor
Thank you!
changed parameter.

@szepeviktor
Copy link
Collaborator

どういたしまして!

@szepeviktor
Copy link
Collaborator

Just add * @template TModelClass of \Illuminate\Database\Eloquent\Model to both docblocks and we are done 🚀

@fagai
Copy link
Contributor Author

fagai commented May 13, 2022

@szepeviktor
OK, Thank you!

@szepeviktor szepeviktor requested a review from canvural May 13, 2022 17:33
Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

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

Hi,

Thank you for this! Just 2 small nitpicks.

stubs/QueryBuilder.stub Show resolved Hide resolved
stubs/QueryBuilder.stub Show resolved Hide resolved
@canvural canvural merged commit 547113a into larastan:master May 23, 2022
@canvural
Copy link
Collaborator

Ignore the previous comments. Github's diff viewer fooled me.

Thank you!

@szepeviktor
Copy link
Collaborator

Github's diff viewer fooled me.

@canvural Actually those were TAB characters.
Please see be2c6b3

@canvural
Copy link
Collaborator

@szepeviktor Ah, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants