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

Feature/belongs to many support #97

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tonyfulls845
Copy link

No description provided.

@fico7489
Copy link
Owner

nice! I will review and merge tomorrow

@fico7489
Copy link
Owner

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 ?

  1. 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')

  1. 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();
}

@tonyfulls845
Copy link
Author

tonyfulls845 commented Apr 14, 2020

Yes i separate PRs.

  1. 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]
)
  1. 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();
}

@fico7489
Copy link
Owner

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]')")

@tonyfulls845
Copy link
Author

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?

@tonyfulls845
Copy link
Author

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.

@fico7489
Copy link
Owner

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]')")

@tonyfulls845
Copy link
Author

tonyfulls845 commented Apr 14, 2020

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]')")

@fico7489
Copy link
Owner

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]')")

@tonyfulls845
Copy link
Author

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.

@tonyfulls845 tonyfulls845 force-pushed the feature/belongs_to_many_support branch from 1676c2b to e9edd6f Compare April 14, 2020 15:43
@tonyfulls845
Copy link
Author

I can't separate start of morph relation because i also use it for testing InvalidRelation exception like you tested belongToMany in current master.

@jirbel
Copy link

jirbel commented Sep 27, 2021

any progress on this? thanks @tonyfulls845 @fico7489

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.

3 participants