Skip to content
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

Adds the EloquentOrderByToLatestOrOldestRector rule #142

Merged
merged 2 commits into from
Sep 22, 2023

Conversation

peterfox
Copy link
Collaborator

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');

@driftingly driftingly merged commit 2960c46 into driftingly:main Sep 22, 2023
4 checks passed
@johnbacon
Copy link
Contributor

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
Copy link
Collaborator Author

peterfox commented Oct 8, 2023

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
Copy link
Contributor

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
Copy link
Collaborator Author

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

@peterfox
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants