laravel-eloquent-join
laravel-eloquent-join copied to clipboard
fico7489
Feature/belongs to many support
I am using this package in my commercial project so it is important to me to keep this package clean.
I see that you mixed 4 things in this PR :
- BelongsToMany support (finished)
- whereRaw and orWhereRaw support (finished)
- useFullPathTableAlias support (finished)
- Polymorphic Relationships (started)
can you create a distinct PR for each of them ?
- for whereRawJoin can you use params same as laravel method whereRaw does ?
public function whereRaw($sql, $bindings = [], $boolean = 'and')
currently you are using :
public function whereRawJoin($column, $sql, $bindings = [], $boolean = 'and')
- and if it is possible can you move all code regarding BelongsToManyJoin here :
if ($relatedRelation instanceof BelongsToJoin) {
//...
} elseif ($relatedRelation instanceof HasOneJoin || $relatedRelation instanceof HasManyJoin) {
//...
} elseif ($relatedRelation instanceof BelongsToManyJoin) {
// -> HERE
} else {
throw new InvalidRelation();
}
Yes i separate PRs.
- Yes, the syntax is different but we should be able to specify a column because in the general case we have a dynamic column name, since the table name is dynamic. in PDO there is no support for column binding, and therefore the only way to make it extensible is to pass the column with a parameter. It’s probably better to even pass an array of columns. If we needs to use multiple columns in a query. What do you think? and interface will be like this
whereRawJoin(
':column1 = ? OR :column1 = ?',
$columnsBindings = ['column1' => 'sellers.city.state'],
$bindings = [1, 2]
)
- Unfortunately, such relation must require join of two tables and a specific set of current table if we try get data in the intermediate table (pivot). And to process case when we have already join pivot and related table and we need data from pivot table we need change $currentTableAlias. And only one place exist for this action outside
if ($relatedRelation instanceof BelongsToJoin) {
//...
} elseif ($relatedRelation instanceof HasOneJoin || $relatedRelation instanceof HasManyJoin) {
//...
} elseif ($relatedRelation instanceof BelongsToManyJoin) {
// -> HERE
} else {
throw new InvalidRelation();
}
I think your solution for 1. is not good enough, because when you need, for example, to filter by JSON your solution would now work....
with
public function whereRaw($sql, $bindings = [], $boolean = 'and')
it would be :
Order::joinRelations('seller')
->whereRawJoin("JSON_CONTAINS(sellers.payment_methods, '["Cash]')")
it turns out it will not be compatible with useTableAlias, and for support useFullPathTableAlias we need use something like this
Order::setUseFullPathTableAlias(true)->joinRelations('item.seller')
->whereRawJoin("JSON_CONTAINS(items_sellers.payment_methods, '["Cash]')")
is it OK for you?
But in my solution everything will work also we will not be connected with the real name of the table, and suddenly we’ll imagine the table name changes and we don’t need to change the code, I think this is more correct.
yeah, whereRaw is very specific, I would definitely go with this interface :
public function whereRaw($sql, $bindings = [], $boolean = 'and')
but I am not sure which solution is the best for above case :
this one is valid for now and it can be merged:
Order::setUseFullPathTableAlias(true)->joinRelations('item.seller')
->whereRawJoin("JSON_CONTAINS(items_sellers.payment_methods, '["Cash]')")
but later I will might go with more elegant solution, maybe something like this :
Order::whereRawJoin("JSON_CONTAINS(relation:items.sellers.payment_methods, '["Cash]')")
I have to hardcode this prefix. I see unobvious behavior in the future and possible conflicts, but in my decision it remains entirely under the control of the developer. dwell on this variant?
Order::whereRawJoin("JSON_CONTAINS(relation:item.sellers.payment_methods, '["Cash]')")
in your PR just use this interface
"public function whereRaw($sql, $bindings = [], $boolean = 'and')"
and then this would work :
Order::setUseFullPathTableAlias(true)->joinRelations('item.seller')
->whereRawJoin("JSON_CONTAINS(items_sellers.payment_methods, '["Cash]')")
this is just for a consideration for the future, not confirmed solution, and not for this PR :
Order::whereRawJoin("JSON_CONTAINS(relation:item.sellers.payment_methods, '["Cash]')")
but this is just an upgrade, with that upgrade the old solution would also work :
Order::setUseFullPathTableAlias(true)->joinRelations('item.seller')
->whereRawJoin("JSON_CONTAINS(items_sellers.payment_methods, '["Cash]')")
Yes, I understand, I just now need to dynamically set the table, because I am doing a very universal API. therefore, I want to implement support for this functionality.
I can't separate start of morph relation because i also use it for testing InvalidRelation exception like you tested belongToMany in current master.
