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

Add relative date shorthands to Query Builder #54408

Merged
merged 9 commits into from
Feb 5, 2025

Conversation

jasonmccreary
Copy link
Contributor

@jasonmccreary jasonmccreary commented Jan 29, 2025

I PR'd a version of this almost 3 years ago and still reach for it. So I figured I'd give it another shot.

This adds a few relative, human readable shorthands for working with dates (cause dates are hard).

$archivedPost = Post::wherePast('archived_at')->get();
$currentReleases = Release::whereToday('released_at')->get();
$activeTokens = Token::whereFuture('expire_at')->get();

While I appreciate this adds weight to this ever growing class, there are dozens of other shorthands. Queries are something devs write everyday. As such, I believe any shorthand continues to improve the DX of Laravel.

All of these methods have their where, or, and orWhereNot counterparts. All methods also accept an array of column names as the first argument. The wherePast and whereFuture have an optional second argument to set $now.

Copy link
Contributor

@shaedrich shaedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say, I like the naming, though. I'd prefer something like whereDateBefore and whereDateAfter or the like 🤔

@selcukcukur
Copy link
Contributor

selcukcukur commented Jan 30, 2025

It would be nice if there was a trait class for date relations like DateRelations which would simplify all date related operations with the least possible repetition using regular expressions. However, the pull request you submitted does not seem like a good solution.

@nexxai
Copy link
Contributor

nexxai commented Jan 30, 2025

I am shocked that there is any resistance to this at all. The changes are small, solve a common paper cut, and don't impose any obvious performance penalties. I am definitely all for this.

@shaedrich
Copy link
Contributor

shaedrich commented Jan 30, 2025

It would be nice if there was a trait class for date relations like DateRelations which would simplify all date related operations with the least possible repetition using regular expressions. However, the pull request you submitted does not seem like a good solution.

@selcukcukur There could be a trait for all where combinations, which are quite a lot already.

I am shocked that there is any resistance to this at all. The changes are small, solve a common paper cut, and don't impose any obvious performance penalties. I am definitely all for this.

@nexxai It adds yet another where method, but no, I'm not generally against it

@tpetry
Copy link
Contributor

tpetry commented Jan 30, 2025

For PostgreSQL this would have to use a full iso-8601 date with timezone.

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented Jan 30, 2025

@tpetry, do you know if the underlying where builder method formats a Carbon object full ISO-8601. My only real concern here is the pre-formatting versus just passing along Carbon::now(). However, the original PR had several comments about using sub-second precision.


Follow-up: A quick dump shows that where('date_column', now()) currently does not format as ISO-8601 with timezone on Postgres. So while you may be right, this PR at least aligns with current date formatting the query builder would do in Postgres (as far as I can tell).

@tpetry
Copy link
Contributor

tpetry commented Jan 30, 2025

Yeah, its not correct currently for PG. I think the best approach would be copying the implementation of substituteBindings so grammar-specific behaviours are accounted for:

if ($value instanceof DateTimeInterface) {
    $bindings[$key] = $value->format($grammar->getDateFormat());
 }

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented Jan 30, 2025

@tpetry, my humble vote would be to merge this and then address that in a separate PR for where (possibly targetting Laravel 12). These methods could then simply defer to where instead of pre-format.

@tontonsb
Copy link
Contributor

tontonsb commented Feb 2, 2025

Most of the ->where* methods are accepting a $column not $columns. Why did you decide to go for multiple here?

Not saying I'm against the feature per se, but I don't understand the need for complexity in the implementation. What's insufficient with the following?

            public function wherePast($column, $boolean = 'and', $not = false)
	    {
	        return $this->where($column, $not ? '>=' : '<', now(), $boolean);
	    }

            public function whereFuture($column, $boolean = 'and', $not = false)
	    {
	        return $this->where($column, $not ? '>' : '<=', now(), $boolean);
	    }

            public function whereToday($column, $boolean = 'and', $not = false)
	    {
	        return $this->whereDate($column, $not ? '<>' : '=', now(), $boolean);
	    }

I'm also a bit afraid this would open up the next need for things like ->whereBeforeToday, ->whereLastWeek and so forth :D

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented Feb 2, 2025

@tontonsb, many where methods allow for multiple columns. In fact, where itself does.

Yes, if multiple columns and micro-second precision is not needed, then as noted in several previous comments, the implementation could be streamlined. However, as is, they are not equivalent.

I'm leaving it up to the team to decide which direction, if any, they want to take. PR comments can get a bit noisy of late.

dry
@taylorotwell taylorotwell merged commit baba544 into laravel:11.x Feb 5, 2025
37 of 38 checks passed
@jasonmccreary jasonmccreary deleted the date-shorthands branch February 6, 2025 19:06
@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented Feb 6, 2025

Looks like this got merged. Yay!

I do my best to maintain and promote the "good vibes" of the Laravel community. But, I have to admit, of late, I have not wanted to endure the comments from submitting a PR. So, I have some specific and general feedback... @shaedrich, you seem to comment early and often on a lot of PRs. That's cool, but can create a lot of noise. @selcukcukur, you can always make your own PR to refactor something after it's merged. Leaving the same comment on PRs is also noise.

Generally, I would suggest sharing your opinion, but remain encouraging and offer suggestions. If something is truly "not a good solution", trust it won't be merged. Don't kill the "good vibes".

@shaedrich
Copy link
Contributor

@shaedrich, you seem to comment early and often on a lot of PRs. That's cool, but can create a lot of noise.

Taylor will most likely not reply to a PR when closing it other than a standard reply. He'll also not read much about thought processes and such. So, I think, its important to get into discussion with the community, offer a different angle, and help shape features.

@DougSisk
Copy link
Contributor

Lack of timezone support for the today related functions seems like a massive oversight. Could push people to move their app timezone away from UTC without understanding the implications of doing so.

@jasonmccreary
Copy link
Contributor Author

@DougSisk, yes, @tpetry commented on something similar for Postgres specifically. Since this simply delegates to the current builder, whatever "oversight" may exist would also exist for all date query builder methods. If you believe that to be the case, I encourage a PR to improve this for Laravel 12.

@tpetry
Copy link
Contributor

tpetry commented Feb 11, 2025

I‘ll work on a PR this week

@patrickomeara
Copy link
Contributor

This is awesome @jasonmccreary! I can immediately use this. Well done. ❤️

@tpetry
Copy link
Contributor

tpetry commented Feb 14, 2025

@jasonmccreary The whereToday and similar methods cast the timestamp to a date for comparison (e.g. DATE(created_at)). This means an index on created_at won't be used - and there is no reason good reason for this? The code could be changed to just do WHERE created_at BETWEEN :startofday AND :endofday and an index will be used. Additionally, the tests don't have to test all different databases anymore.

Can you fix it or should i?

@jasonmccreary
Copy link
Contributor Author

@tpetry, I am not a core team member, so I can't speak to what (if any) changes will be made or who would make them.

As I've noted, if you (or anyone) feel something about the framework can be improved I'd encourage you to open your own PR. This PR has been merged. I'm happy with the new functionality, and it matches how other similar methods currently behave. Again, if you feel that behavior should change, submit a PR. 👍

@debuGhy
Copy link

debuGhy commented Feb 27, 2025

This will help in making our code more cleaner. Good to see the new feature.

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

10 participants