Skip to content

Conversation

fagai
Copy link

@fagai fagai commented May 13, 2022

The union method is supports EloquentBuilder, but an error occurs because there is no description.

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

$users = User::where('id',2)->union($first)->get();
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.

@fagai fagai changed the title PHPStan support for union methods [9.x]PHPStan support for union methods May 13, 2022
@GrahamCampbell GrahamCampbell changed the title [9.x]PHPStan support for union methods [9.x] PHPStan support for union methods May 13, 2022
@GrahamCampbell
Copy link
Member

This is not necessary. Including subtypes of types already in the union adds no extra information. It like having a the type Person and then having Person|Man. The types are already identical, and |Man should be removed.

@fagai
Copy link
Author

fagai commented May 13, 2022

@GrahamCampbell
Illuminate\Database\Eloquent\Builder does not inherit from Illuminate\Database\Query\Builder.
It didn't look like a subtype, but is it different?

https://github.com/laravel/framework/blob/9.x/src/Illuminate/Database/Eloquent/Builder.php#L30
https://github.com/laravel/framework/blob/9.x/src/Illuminate/Database/Query/Builder.php#L29

@@ -2438,7 +2438,7 @@ protected function removeExistingOrdersFor($column)
/**
* Add a union statement to the query.
*
* @param \Illuminate\Database\Query\Builder|\Closure $query
* @param \Illuminate\Database\Query\Builder|\Illuminate\Database\Eloquent\Builder|\Closure $query
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct, for the reason @GrahamCampbell mentioned:

This is not necessary. Including subtypes of types already in the union adds no extra information.

Copy link
Author

Choose a reason for hiding this comment

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

Each class implements a different interface, so they look different.
Am I misunderstanding?

Copy link
Contributor

@zonuexe zonuexe May 13, 2022

Choose a reason for hiding this comment

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

@nunomaduro
Illuminate\Database\Eloquent\Builder and Illuminate\Database\Eloquent\Builder are not subclassed to each other, so they are not what @GrahamCampbell calls Person|Man.

The code that @fagai exemplifies in the issue is valid code that actually works.

Is there a reasonable reason to exclude Illuminate\Database\Eloquent\Builder from PHPDoc?

@n1215
Copy link

n1215 commented May 13, 2022

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

@fagai
Copy link
Author

fagai commented May 13, 2022

@GrahamCampbell @nunomaduro
Not a subtype...

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.

6 participants