[8.x] BuildsQueries::tap() doesn't always return $this despite being documented as such

[axlon]

• Laravel Version: 8.53.0 (I first spotted this in 7.x, but it may have been around for longer)
• PHP Version: 8.0.8
• Database Driver & Version: n/a

Description:

The BuildsQueries::tap() method is documented to return $this, but in reality does not always return $this or even a builder instance at all. This method calls (and returns) Conditionable::when() which returns the return value of the given callback if it is truthy, if the return value is not truthy it will return $this.

This leads to situations where a query chain may break in a type error.

By contrast, this is the contents of EnumeratesValues::tap() (which is used by collections):

    public function tap(callable $callback)
    {
        $callback(clone $this);

        return $this;
    }

Collection's implementation always return $this regardless of the callback's return value.

Another example is the global tap() helper:

    function tap($value, $callback = null)
    {
        if (is_null($callback)) {
            return new HigherOrderTapProxy($value);
        }

        $callback($value);

        return $value;
    }

This function can also return a proxy when there is no callback, but when there is a callback it's return value is always ignored and $value is returned.

(Previous PR about this issue #32717 on 7.x)

Steps To Reproduce:

1. Create a new Laravel project and run the migrations
2. Paste the following snippet in the default route and open the project in your browser:

// The first time you run this it should work without much issue,
// after confirming the code does not break uncomment the line below:
//
//    \Database\Factories\UserFactory::new()->createOne();

    \App\Models\User::query()
        ->tap(fn ($query) => $query->update(['name' => 'Taylor Otwell']))
        ->count();

    return 'It works!';

How to fix:

I see two ways to fix this:

• Accept that this is how this method works, in which case I believe it's return type should be adjusted to include mixed ($this is part of mixed, but making it mixed only would break auto completion for a lot of people)
• Fix this method to be consistent with other versions of tap, while it "technically" would not be a breaking change as this function should only return $this as is, it would probably be best to make this change in 9.x to avoid breaking any projects relying on current behavior

Personally I think the second fix would be the way to go, but I'm willing to provide a PR for either change if accepted.

[driesvints]

Thanks for the thorough issue. I've sent in a PR with the first suggestion: #38353