-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Potentional bug with ->month() && ->setMonth()
Overflow vs. noOverflow
#2317
Comments
Hello 👋 This is a duplicate of #2309, #2260, #2176
This behavior cannot change on a minor, as it would be a breaking change. In different cases, we might expect the overflow behavior to ensure But I will soon work more actively on 3.x so there is room to reconsider this at this point. |
For the record, this also behave consistently with JavaScript date = new Date('2021-01-31');
date.setMonth(1); // (index from January = 0)
console.log(date); Output: Which is inherited from Java which has the exact same behavior. I will make some tour of the behavior for each language but I fear than having noOverflow by default would make Carbon an inconsistent outsider in the pool. |
I am now closing the scope of breaking changes for 3.0.0. Here is to summarize how to deal with such cases when you face them. Taking the example of: Carbon::parse('2024-03-31 23:00:00')->month(2); There are various possible expectations for the date part:
Then various possible expectations on the time part:
So way too much combos, and the default one PHP and JS pick: In early versions
This would force the developer to take a explicit decision and so taking account for the context of its application such as changing code for one of the following: $origin = Carbon::parse('2024-03-31 23:00:00');
try {
$date = $origin->month(2);
} catch (OverflowException) {
$date = $origin->modify('Last day of February');
// or
$date = $origin->modify('First day of March');
// or
$date = $origin->modify('February'); // Enforces the native PHP overflow behavior
// etc.
} But this still has some dangerous pitfalls, while in some cases having a wrong date/silent bug is more harmful than having a show-stopper exception. In some other cases, I mainly thinking about e-commerce/subscription, you might prefer to secure a transaction to happen without interruption for the user, then detect and fix anomalies in a background task. Also if the method is called on "now", possible issues are discoverable only on the 31st or if the thing is properly tested with mocking time that actually test this edge-case. An option could be to enable such exception with a particular setting (would looks like: So as a result, nothing will change for the release of 3.0.0 around month overflow. However, I'm thinking about some set of tools for carbonite that could help to detect those edge-cases and provide some guidance |
Hello,
I encountered an issue with the following code:
I reckon this should use
noOverflow
behind the scenes instead ofoverflow
Carbon version: v2.24.0
PHP version: 7.3.23
I expected to get:
But I actually get:
Thanks!
The text was updated successfully, but these errors were encountered: