search

hammer-tools / laravel-hammer

Apache License 2.0
10 stars 1 forks source link

Change Builder implementation to interface #2

Open DrowningElysium opened 1 year ago

DrowningElysium commented 1 year ago

Scopes require the Contract in my opinion, because of when you call a scope from a relation directly.

Example: $model->relation()->active();

This will return an error as the active() is called on the Relation class instead of the Eloquent Builder, which is not extended by it.

rentalhost commented 1 year ago

Thanks for PR. Can you examplify it a little bit? I'm having a difficulty to understanding 100% how your PR can improve the code. I confess that I don't usually use Contracts a lot.

Perhaps you are suggesting that we can use the Contract class as the parameter type instead of the concrete class? Would this work well for Laravel scopes? If yes, I can accept your PR.

DrowningElysium commented 1 year ago

Sorry for the late reply. Forgot about the email.

When building a relation in Laravel you have the Relations class. This is what comes from $this->belongsTo(...), $this->hasMany(...), etc. These classes, that you get from these functions, do not extend \Illuminate\Database\Eloquent\Builder. They instead implement \Illuminate\Contracts\Database\Eloquent\Builder. See here line 6 and 16.

In the example you can see that the active scope is called on the relation instead of the model. Which passes on the Relation object instead of a Eloquent Builder object. This is why I suggest changing the Builder class type hinting in scopes. As in the old situation you will get an error when calling a scope on a relation directly.