8000 [8.x] Update return type for tap by driesvints · Pull Request #38353 · laravel/framework · GitHub
[go: up one dir, main page]
Skip to content

[8.x] Update return type for tap #38353

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 12, 2021
Merged

[8.x] Update return type for tap #38353

merged 1 commit into from
Aug 12, 2021

Conversation

driesvints
Copy link
Member

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

@driesvints
Update return type for tap
dd6349c 
`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
@driesvints driesvints mentioned this pull request Aug 12, 2021
Closed
@axlon
Copy link
Contributor
axlon commented Aug 12, 2021

Thanks for creating the PR @driesvints!

Do you think we should alter this function's behaviour on 9.x to be the same as Collection::tap() and tap()?

@driesvints
Copy link
Member Author

@axlon that's for @taylorotwell to decide.

@taylorotwell taylorotwell merged commit f67efea into 8.x Aug 12, 2021
@taylorotwell
Copy link
Member

Merging this for now but this tap method is not correct. It should always return $this so it has the same behavior as the tap helper.

@taylorotwell taylorotwell deleted the driesvints-patch-1 branch August 12, 2021 12:47
@axlon axlon mentioned this pull request Aug 12, 2021
Merged
@HazzazBinFaiz HazzazBinFaiz mentioned this pull request Jun 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@driesvints @axlon @taylorotwell
0