Skip to content

Conversation

@DrowningElysium
Copy link

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.

Scopes require the Contract in my opinion, because of when you call a scope from a relation directly.
Example: 
```$model->relation()->active();```
will return an error as the active() is called on the Relation class instead of the Eloquent Builder, which is not extended.
@rentalhost
Copy link

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.

@rentalhost rentalhost added the enhancement New feature or request label Feb 13, 2023
@DrowningElysium
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants