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

[11.x] Fix query builder whereBetween with CarbonPeriod and Carbon 3 #50792

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

bakerkretzmar
Copy link
Contributor

This PR fixes the query builder's whereBetween() and havingBetween() methods by calling getStartDate() and getEndDate() on CarbonPeriod, instead of using its $start and $end properties. Currently, using Carbon 3, $period->start is always 2000-01-01 and $period->end is always null (see briannesbitt/Carbon#2982).

In Carbon 2 this is actually exactly what $start and $end already do—they are not defined, so they go through the CarbonPeriod class's __get() method, which calls getStartDate() and getEndDate() (https://github.com/briannesbitt/Carbon/blob/2.x/src/Carbon/CarbonPeriod.php#L757).

In Carbon 3 those getters are all still set up exactly the same way, but they don't work (I assume this was unintentional). CarbonPeriod now extends PHP's DatePeriod class, which actually defines $start and $end properties, so the getters are never called.

@kylekatarnls
Copy link
Contributor

kylekatarnls commented Mar 27, 2024

In Carbon 3 those getters are all still set up exactly the same way

No, they are no longer getters because readonly properties are set in the parent DatePeriod constructor so we don't have control of them in the sub-class.

In Carbon v2 CarbonPeriod was not extending DatePeriod (because PHP didn't allow it properly before 8.1)

I'm trying to rethink the CarbonPeriod constructor so to have them set at creation. But using getStartDate() and getEndDate() would be a good move anyway. BTW those methods also exist on DatePeriod so it could be supported too.

And another note: DatePeriod (and CarbonPeriod) don't always have an end, it can have a number of recurrences instead.

@bakerkretzmar
Copy link
Contributor Author

No, they are no longer getters because readonly properties are set in the parent DatePeriod constructor so we don't have control of them in the sub-class.

Yeah that's more or less what I meant—the getters don't work that way anymore but looking at the code it seems like they were intended to, because the mappings for start, end, etc. are still listed where all the getters are handled.

BTW those methods also exist on DatePeriod so it could be supported too.

Cool, didn't realize that! whereBetween() can probably be updated to support that too then yeah.

@taylorotwell taylorotwell merged commit 488e251 into laravel:11.x Mar 27, 2024
28 checks passed
@bakerkretzmar bakerkretzmar deleted the fix-between-carbon-period branch March 27, 2024 22:22
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.

3 participants