-
-
Notifications
You must be signed in to change notification settings - Fork 859
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
[9.0] Add support for self join relationships. #2051
Conversation
…ith assertStringContainsString.
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.
The PR looks good with some DB compatibility issues. Will try to test this on actual project and get back to this as soon as I can. Thanks!
src/EloquentDataTable.php
Outdated
@@ -169,19 +169,23 @@ protected function joinEagerLoadedColumn($relation, $relationColumn) | |||
|
|||
case $model instanceof BelongsTo: | |||
$table = $model->getRelated()->getTable(); | |||
$alias = "alias_{$index}_{$table}"; | |||
$tableAs = "{$table} AS $alias"; |
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.
This will break some installations with Oracle database. Please use "{$table} $alias"
removing the AS
from query.
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.
But if i remove the AS
then MySQL won't work... ok, i managed something, update coming soon.
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.
Can you run your tests again now?
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.
Sorry but your previous PR version is much better. AFAIK, it should work on MySQL since that is one of the reasons why I created this package version to work on Oracle.
On the other hand, I think I already created a patch that properly translates Oracle AS
query. Will review with my other package https://github.com/yajra/laravel-oci8 too.
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.
So, what now? You want the first one again?
src/EloquentDataTable.php
Outdated
@@ -38,6 +45,9 @@ public function __construct($model) | |||
parent::__construct($builder->getQuery()); | |||
|
|||
$this->query = $builder; | |||
|
|||
$connection = config('database.default'); |
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.
There are cases where the connection is set from the model or via runtime using query. This is will not work on those situations.
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.
Ok, sure. But in your earlier reply (#2051 (comment)) you said, basically, this wouldnt even be necessary. So, what now?
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.
Yes, so please revert the commits to 1st one you sent. Might be able to test this within the week. Thanks!
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.
Ok, done. Sorry for the mess with the commits.
please consider the soft delete , the new release cause an error "cloumn related_table.deleted_at not fount in where clause" however it shall be related_table_0_deleted_at I rollback back to 9.0.1 and it works just perfect . thank you for the great work , this project helps me a lot making fast delevlopment ! |
Reverting due to unseen breaking change on soft deletes. Related issue: #2058 |
Any news on this? |
@yajra is this issue already addressed in the latest version or is it still going on? |
This fix not working |
public function query(Jobseeker $model): QueryBuilder //this is the parameter i'm passing for column search this is my query for datatable it is working perfect but search is not working on relational data |
A fix for the issue #2045 . It only addresses the "BelongsTo". I did not touch the rest of the relations, so a more complete "fix" can be made.
I also replaced the deprecated function "assertContains" with "assertStringContainsString" in the QueryDataTableTest.
I ran the tests and nothing exploded :P