Skip to content

Conversation

@axlon
Copy link
Contributor

@axlon axlon commented May 7, 2020

This changes the documented return type of BuildsQueries::tap() to $this, which makes it so Eloquent\Builder::tap() doesn't report it's return type as Query\Builder. The reported return type will now be the exact same as BuildsQueries::when() which this method proxies.

@taylorotwell
Copy link
Member

But when and tap actually do have different return types.

@axlon
Copy link
Contributor Author

axlon commented May 7, 2020

@taylorotwell But BuildsQueries::tap() always ends up here which returns either the output of the given callback (mixed) or $this if the callback output is falsey.

Which would mean that for the eloquent builder the return value is actually never a "normal" query builder unless the callback returns one, or am I missing something here?

driesvints added a commit that referenced this pull request Aug 12, 2021
`when` already has `$this|mixed` so this should be reflected in `tap` on `BuildsQueries`. If you look at the logic inside `when` you'll see that this is the correct return type as it's either the return value of the given callback or `$this`. A previous attempt was made here: #32717

See a more thorough explanation here: #38343
taylorotwell pushed a commit that referenced this pull request Aug 12, 2021
`when` already has `$this|mixed` so this should be reflected in `tap` on `BuildsQueries`. If you look at the logic inside `when` you'll see that this is the correct return type as it's either the return value of the given callback or `$this`. A previous attempt was made here: #32717

See a more thorough explanation here: #38343
taylorotwell pushed a commit to illuminate/database that referenced this pull request Aug 12, 2021
`when` already has `$this|mixed` so this should be reflected in `tap` on `BuildsQueries`. If you look at the logic inside `when` you'll see that this is the correct return type as it's either the return value of the given callback or `$this`. A previous attempt was made here: laravel/framework#32717

See a more thorough explanation here: laravel/framework#38343
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.

2 participants