Skip to content

Commit 85958eb

Browse files
AlexVanSteenhovenAlexVanSteenhoven
committed
fix attribute sorting
1 parent 7429032 commit 85958eb

15 files changed

+1531
-149
lines changed

‎.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,5 @@ phpstan.neon
3030
testbench.yaml
3131
/docs
3232
/coverage
33+
34+
.cursor

‎src/Concerns/HasColumns.php‎

Lines changed: 186 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -143,41 +143,117 @@ public function renderCell(Column $column, mixed $record): mixed
143143
return $value;
144144
}
145145

146+
/**
147+
* Cache for attribute detection to avoid repeated reflection calls.
148+
*
149+
* @var array<string, array<string, bool>>
150+
*/
151+
protected static array $attributeDetectionCache = [];
152+
146153
/**
147154
* Check if a field is a model attribute (accessor) rather than a database column.
155+
* Enhanced version with improved detection and caching for performance.
148156
*/
149157
protected function isModelAttribute(\Illuminate\Database\Eloquent\Model $model, string $field): bool
150158
{
151-
// Check if it's an accessor method (old Laravel syntax)
152-
$accessorMethod = 'get' . \Illuminate\Support\Str::studly($field) . 'Attribute';
153-
if (method_exists($model, $accessorMethod)) {
154-
return true;
155-
}
159+
$modelClass = get_class($model);
160+
$cacheKey = $modelClass . '::' . $field;
156161

157-
// Check if it's defined in the model's $appends array
158-
if (in_array($field, $model->getAppends())) {
159-
return true;
162+
// Return cached result if available
163+
if (isset(static::$attributeDetectionCache[$modelClass][$field])) {
164+
return static::$attributeDetectionCache[$modelClass][$field];
160165
}
161166

162-
// Check if it's a cast attribute
163-
if (array_key_exists($field, $model->getCasts())) {
164-
return true;
165-
}
167+
$isAttribute = false;
166168

167-
// Check if it's a Laravel 9+ Attribute (new syntax)
168-
if (method_exists($model, $field)) {
169-
$reflection = new \ReflectionClass($model);
170-
if ($reflection->hasMethod($field)) {
171-
$method = $reflection->getMethod($field);
172-
$returnType = $method->getReturnType();
169+
try {
170+
// 1. Check if it's an accessor method (old Laravel syntax)
171+
$accessorMethod = 'get' . \Illuminate\Support\Str::studly($field) . 'Attribute';
172+
if (method_exists($model, $accessorMethod)) {
173+
$isAttribute = true;
174+
}
173175

174-
if ($returnType instanceof \ReflectionNamedType && $returnType->getName() === 'Illuminate\Database\Eloquent\Casts\Attribute') {
175-
return true;
176+
// 2. Check if it's defined in the model's $appends array
177+
if (!$isAttribute && in_array($field, $model->getAppends())) {
178+
$isAttribute = true;
179+
}
180+
181+
// 3. Check if it's a cast attribute but NOT a database column
182+
if (!$isAttribute && array_key_exists($field, $model->getCasts())) {
183+
// Cast attributes can be both database columns AND computed attributes
184+
// We need to check if it's actually a database column
185+
try {
186+
if (!$this->isDatabaseColumn($model, $field)) {
187+
$isAttribute = true;
188+
}
189+
} catch (\Throwable $e) {
190+
// If we can't determine database columns, assume it's an attribute if it's cast
191+
$isAttribute = true;
192+
}
193+
}
194+
195+
// 4. Check if it's a Laravel 9+ Attribute (new syntax)
196+
if (!$isAttribute && method_exists($model, $field)) {
197+
try {
198+
$reflection = new \ReflectionClass($model);
199+
if ($reflection->hasMethod($field)) {
200+
$method = $reflection->getMethod($field);
201+
$returnType = $method->getReturnType();
202+
203+
if ($returnType instanceof \ReflectionNamedType &&
204+
$returnType->getName() === 'Illuminate\Database\Eloquent\Casts\Attribute') {
205+
$isAttribute = true;
206+
}
207+
}
208+
} catch (\ReflectionException $e) {
209+
// Log the error but don't fail
210+
\Illuminate\Support\Facades\Log::debug('Reflection error in attribute detection', [
211+
'model' => $modelClass,
212+
'field' => $field,
213+
'error' => $e->getMessage()
214+
]);
176215
}
177216
}
217+
218+
// 5. Check for computed attributes that use getAttribute() with custom logic
219+
// Disabled for now to avoid false positives - the above checks should be sufficient
220+
// if (!$isAttribute) {
221+
// try {
222+
// // First check if it's a database column - if so, it's not a computed attribute
223+
// if ($this->isDatabaseColumn($model, $field)) {
224+
// $isAttribute = false;
225+
// } else {
226+
// // Create a fresh model instance to test attribute access
227+
// $testModel = new $modelClass;
228+
// $testModel->exists = true; // Prevent save operations
229+
//
230+
// // Try to access the attribute - if it exists and isn't a database column, it's computed
231+
// if ($testModel->hasGetMutator($field) || $testModel->hasAttributeMutator($field)) {
232+
// $isAttribute = true;
233+
// }
234+
// }
235+
// } catch (\Throwable $e) {
236+
// // Ignore errors in this detection method - it's a fallback
237+
// }
238+
// }
239+
240+
} catch (\Throwable $e) {
241+
// Log error and return false as fallback
242+
\Illuminate\Support\Facades\Log::warning('Error in attribute detection', [
243+
'model' => $modelClass,
244+
'field' => $field,
245+
'error' => $e->getMessage()
246+
]);
247+
$isAttribute = false;
178248
}
179249

180-
return false;
250+
// Cache the result
251+
if (!isset(static::$attributeDetectionCache[$modelClass])) {
252+
static::$attributeDetectionCache[$modelClass] = [];
253+
}
254+
static::$attributeDetectionCache[$modelClass][$field] = $isAttribute;
255+
256+
return $isAttribute;
181257
}
182258

183259
/**
@@ -229,9 +305,95 @@ protected function getModelAttributeValue(\Illuminate\Database\Eloquent\Model $m
229305
*/
230306
protected function isDatabaseColumn(\Illuminate\Database\Eloquent\Model $model, string $field): bool
231307
{
232-
$schema = \Illuminate\Support\Facades\Schema::connection($model->getConnectionName());
233-
$tableColumns = $schema->getColumnListing($model->getTable());
308+
try {
309+
$connectionName = $model->getConnectionName();
310+
$tableName = $model->getTable();
311+
312+
$schema = \Illuminate\Support\Facades\Schema::connection($connectionName);
313+
$tableColumns = $schema->getColumnListing($tableName);
314+
315+
return in_array($field, $tableColumns);
316+
} catch (\Throwable $e) {
317+
// Log error and return false as fallback
318+
\Illuminate\Support\Facades\Log::warning('Error checking database column', [
319+
'model' => get_class($model),
320+
'field' => $field,
321+
'error' => $e->getMessage()
322+
]);
323+
return false;
324+
}
325+
}
326+
327+
/**
328+
* Check if a relationship field (dot notation) contains a model attribute.
329+
* For example: 'user.full_name' where 'full_name' is an accessor on the User model.
330+
*/
331+
protected function isRelationshipAttribute(string $relationshipPath): bool
332+
{
333+
$parts = explode('.', $relationshipPath);
334+
if (count($parts) < 2) {
335+
return false;
336+
}
337+
338+
try {
339+
$model = $this->getModel();
340+
$relationName = $parts[0];
341+
$relationField = $parts[1];
342+
343+
// Check if the relation method exists
344+
if (!method_exists($model, $relationName)) {
345+
return false;
346+
}
347+
348+
// Get the related model
349+
$relatedModel = $this->getRelatedModel($relationshipPath);
350+
if (!$relatedModel) {
351+
return false;
352+
}
353+
354+
// Check if the field is an attribute on the related model
355+
return $this->isModelAttribute($relatedModel, $relationField);
356+
357+
} catch (\Throwable $e) {
358+
// Log error and return false as fallback
359+
\Illuminate\Support\Facades\Log::warning('Error in relationship attribute detection', [
360+
'relationshipPath' => $relationshipPath,
361+
'error' => $e->getMessage()
362+
]);
363+
return false;
364+
}
365+
}
234366

235-
return in_array($field, $tableColumns);
367+
/**
368+
* Enhanced method to check if a field (with or without dot notation) is a model attribute.
369+
* This is the main entry point for attribute detection.
370+
*/
371+
public function isFieldAttribute(string $field): bool
372+
{
373+
// Handle relationship fields (dot notation)
374+
if (str_contains($field, '.')) {
375+
return $this->isRelationshipAttribute($field);
376+
}
377+
378+
// Handle direct model attributes
379+
try {
380+
$model = $this->getModel();
381+
return $this->isModelAttribute($model, $field);
382+
} catch (\Throwable $e) {
383+
\Illuminate\Support\Facades\Log::warning('Error in field attribute detection', [
384+
'field' => $field,
385+
'error' => $e->getMessage()
386+
]);
387+
return false;
388+
}
389+
}
390+
391+
/**
392+
* Clear the attribute detection cache.
393+
* Useful for testing or when model definitions change at runtime.
394+
*/
395+
public static function clearAttributeDetectionCache(): void
396+
{
397+
static::$attributeDetectionCache = [];
236398
}
237399
}

‎src/Concerns/HasSorting.php‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ public function sortBy(string $field): void
5454

5555
/**
5656
* Apply sorting to the query.
57+
* Enhanced version with attribute detection to prevent SQL errors.
5758
*
5859
* @param Builder<\Illuminate\Database\Eloquent\Model> $query
5960
* @return Builder<\Illuminate\Database\Eloquent\Model>
@@ -75,11 +76,23 @@ public function applySorting(Builder $query): Builder
7576
return $callback($query, $sortDirection);
7677
}
7778

78-
$relationship = null;
79+
// Determine the actual field to sort by
7980
if ($column) {
8081
$sortField = $column->getSortField();
82+
}
83+
84+
// Check if we're dealing with a model attribute before proceeding with SQL sorting
85+
if (method_exists($this, 'isFieldAttribute') && $this->isFieldAttribute($sortField)) {
86+
// This field is a model attribute - flag for PHP-based sorting
87+
$this->requiresAttributeSorting = true;
88+
89+
// Return the query unchanged - Table class will handle attribute sorting
90+
return $query;
91+
}
8192

82-
// Check if the field contains dot notation (indicating a relationship)
93+
// Handle relationship fields
94+
$relationship = null;
95+
if ($column) {
8396
$columnField = $column->getField();
8497
if (str_contains($columnField, '.')) {
8598
$relationship = $columnField;
@@ -94,6 +107,7 @@ public function applySorting(Builder $query): Builder
94107
return $this->applySortingWithRelationship($query, $relationship, $sortDirection);
95108
}
96109

110+
// Apply regular SQL sorting for database columns
97111
if (! str_contains($sortField, '.')) {
98112
$sortField = $query->getModel()->getTable() . '.' . $sortField;
99113
}

0 commit comments

Comments
 (0)