-
Notifications
You must be signed in to change notification settings - Fork 68
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
Ability to check exists through tables with "through" key. #30
base: master
Are you sure you want to change the base?
Conversation
Checking exists changed to join, since it's insanely much faster (from tens or hundreds of seconds to milliseconds). Checking not exists is as before. Through works with unlimited number of pivot tables. Example: 'join2through' => array( 'from_table' => 'master2', 'from_col' => 'm2_col', 'to_table' => 'subtable2', 'to_col' => 's2_col', 'to_value_column' => 's2_value', 'not_exists' => true, 'through' => array( 'from_table' => 'subtable2', 'from_col' => 's2_col', 'to_table' => 'subtable3', 'to_col' => 's3_col', 'to_value_column' => 's3_value', ) ), Keep adding "through" if there's more tables in between.
The failed tests comes from the fact that the tests check for exists, instead of how it's done now. I didn't want to rewrite the tests, in case you're not going to accept this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, this is looking good. There are a few changes that I would prefer to be performed before accepting the changes.
Also, please ensure that the tests pass (of course!)
🎉 Thanks for your first contribution to this project!
$condition, | ||
$not | ||
); | ||
if ( $not ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that I am cautious of is that this code looks like it will change the behavior for other users of this class.
Can we separate out the code inside the if() {...}
block into two separate functions to ensure that the length of this function (buildSubclauseQuery
) is not too long?
} else { | ||
// Create a join clause to join to the other table, and find results matching the criteria | ||
|
||
$query = $query->join( $subclause["to_table"], $subclause['to_table'] . '.' . $subclause['to_col'], '=', $subclause['from_table'] . '.' . $subclause['from_col'] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to keep lines < 80 characters long. This line of code wraps in my browser.
@@ -328,4 +343,17 @@ public function testDateNotBetween() | |||
$this->assertEquals(22, $bindings[0]->day); | |||
$this->assertEquals(28, $bindings[1]->day); | |||
} | |||
|
|||
public function testJoinNotExistsInThrough() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, with this project, we are trying to stick to PSR-2
styling (https://www.php-fig.org/psr/psr-2/). Could you please ensure that you are using spaces and not tabs to format code?
Checking exists changed to join, since it's insanely much faster (from tens or hundreds of seconds to milliseconds). Checking not exists is as before.
Through works with unlimited number of pivot tables.
Example:
'join2through' => array(
'from_table' => 'master2',
'from_col' => 'm2_col',
'to_table' => 'subtable2',
'to_col' => 's2_col',
'to_value_column' => 's2_value',
'not_exists' => true,
'through' => array(
'from_table' => 'subtable2',
'from_col' => 's2_col',
'to_table' => 'subtable3',
'to_col' => 's3_col',
'to_value_column' => 's3_value',
)
),
Keep adding "through" if there's more tables in between.