Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork62
Apply camelCase to assumed column name attrib if it contains relation#123
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:4.0
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
component of the name attribute, if the name has not been explicitlyspecified, as the Laravel convention is for Eloquent relation namingto use camelCase formatting.
| @@ -43,14 +52,14 @@ public function columns(array $columns) | |||
| if (is_array($value)) { | |||
| $attributes = array_merge( | |||
| [ | |||
| 'name' => $value['name'] ?? $value['data'] ?? $key, | |||
| 'name' => $value['name'] ?? $this->camelRelation($value['data'] ?? $key), | |||
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.
I think this is not needed on this part since not all name are on relation. This might fail if the column is indeed have underscores on it? I think it's better to do a camelCase on the part where eagerLoading is being checked.
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.
Or this might also fail if the query uses join statements? Anyways, will also try to check on actual projects when I got chance. 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.
Well, the camelRelation function only applies the camel case to the relation part of the name. For example, camelRelation('last_name') -> 'last_name', but camelRelation('job_contacts.last_name') -> 'jobContacts.last_name'
And of course if the 'name' is actually specified in the columns array to begin with, it will not be altered.
I will see if I can write some test classes for this when I get a chance, as I think it would really help to show things like the query join situations you mention.
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, a test would be great. For join queries, it is advised to includetable.column when setting the name which is why this might become a potential breaking change.
When database columns contain values that include a relation with snake case, e.g. "child_table.column", and the Eloquent model has the relation named using the Laravel default of camel case (e.g. childTable ).. the datatable global search will fail because the search query builder fails to find "child_table" within the EagerLoads for the model as it is named in the EagerLoad array as "childTable", leading to SQL query failure because the query does not add the required sub-query.
The docs athttps://datatables.yajrabox.com/eloquent/relationships note this can be avoided by explicitly specifying the name attribute separately from the value, but this PR offers that the column building routine should assume camelCase for relations when no name attribute is explicitly specified in the columns array, as the majority of projects probably will follow that convention.