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

[5.4] Move "tap" method to BuildsQueries trait. #20384

Merged
merged 2 commits into from
Aug 4, 2017
Merged

[5.4] Move "tap" method to BuildsQueries trait. #20384

merged 2 commits into from
Aug 4, 2017

Conversation

ed-fruty
Copy link
Contributor

@ed-fruty ed-fruty commented Aug 2, 2017

Method tap was moved from Illuminate\Database\Query\Builder to Illuminate\Database\Concerns\BuildsQueries trait.

Eloquent builder when and tap methods passes different builder instances to the callback.
when passes current eloquent builder to the callback, but tap method uses Illuminate\Database\Query\Builder as callback argument.

For example:

>>> (new User)->newQuery()->tap(function($builder) { dd(get_class($builder)); });
"Illuminate\Database\Query\Builder"

>>> (new User)->newQuery()->when(true, function($builder) { dd(get_class($builder)); });
"Illuminate\Database\Eloquent\Builder"

So, we can't use one type hint callback for when/tap methods, like (class callback case will illustrate this better):

class SomeBuilderCallback
{
   public function __invoke(Illuminate\Database\Eloquent\Builder $builder)
   {
       // do stuff
   }
}

// In one part of application
User::when($condition, new SomeBuilderCallback);

// In other part
User::tap(new SomeBuilderCallback); // will fail

@ed-fruty ed-fruty changed the title Move "tap" method to BuildsQueries trait. [5.4] Move "tap" method to BuildsQueries trait. Aug 2, 2017
@taylorotwell
Copy link
Member

I don't understand what bug this is fixing.

@ed-fruty
Copy link
Contributor Author

ed-fruty commented Aug 2, 2017

@taylorotwell , so is it normal behavior when i get different builder instances in the similar methods ?

$query->when(true, function ($builder) {

   // Instance of Illuminate\Database\Eloquent\Builder

})->tap(function ($builder) {

  // Another builder instance. Illuminate\Database\Query\Builder

});

Why not one builder instance in the above methods?

@ed-fruty
Copy link
Contributor Author

ed-fruty commented Aug 2, 2017

@taylorotwell , in addition, with tap method i can't use eloquent builder features, like

Model::tap(function ($builder) {
   /*
    * This will throw an exception
    * BadMethodCallException with message 'Call to undefined method Illuminate\Database\Query\Builder::with()'
    */
   $builer->with('relation');

});

@ed-fruty ed-fruty mentioned this pull request Aug 4, 2017
@taylorotwell taylorotwell reopened this Aug 4, 2017
@taylorotwell taylorotwell merged commit 4ce8d7a into laravel:5.4 Aug 4, 2017
This pull request was closed.
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