driftingly / rector-laravel

Rector upgrades rules for Laravel
http://getrector.org
MIT License
500 stars 49 forks source link

Adds the EloquentOrderByToLatestOrOldestRector rule #142

Closed peterfox closed 11 months ago

peterfox commented 11 months ago

Created a new rule for swapping out orderBy or orderByDesc to use latest or oldest instead.

EloquentOrderByToLatestOrOldestRector

Changes orderBy() to latest() or oldest()

 use Illuminate\Database\Eloquent\Builder;

-$builder->orderBy('created_at');
-$builder->orderBy('created_at', 'desc');
-$builder->orderBy('deleted_at');
+$builder->latest();
+$builder->oldest();
+$builder->latest('deleted_at');
johnbacon commented 11 months ago

These changes apply to all columns in our project, not just date/datetime columns. Is that intended behavior? If so, I'm wondering if rector-laravel is open to configuration, so we can specify which columns should and shouldn't have these rules applied.

peterfox commented 11 months ago

The latest and oldest methods work with other columns as well. But yeah, it could be configured to have a whitelisted column configuration option.

johnbacon commented 11 months ago

We've also noticed that this rule changes orderBy() to latest(), when the default orderBy() is ascending:

                ->select('table_name', 'column_name', 'column_type', 'column_default', 'is_nullable', 'ordinal_position', 'column_comment')
                ->where('table_schema', $db)
                ->where('column_name', $like ? 'LIKE' : '=', $column)
-               ->orderBy('table_name')
-               ->orderBy('column_name')
+               ->latest('table_name')
+               ->latest('column_name')

This isn't too bad in our case, as it's an Artisan command. But it could be in others. As an engineer on our team said:

I noticed that did clobber the orderBy() query builder function in my artisan command, which changed the sort order from asc to desc. Changing the logic like that is very undesirable 99% of the time, but in this particular case, it actually worked out - given that this is console output it does make more sense to put the largest last rather than first so one doesn't have to scroll back through however many lines to get back to the top. Good outcome for the wrong reason!

peterfox commented 11 months ago

@johnbacon ah, I see, I'll try to get a fix for that soon.

peterfox commented 11 months ago

@johnbacon I've got a fix for it with #146