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

Potentional bug with ->month() && ->setMonth() Overflow vs. noOverflow #2317

Open
oliversiket opened this issue Apr 14, 2021 · 3 comments
Open
Labels
duplicate Solution/explanation on this issue has already been given in an other issue
Milestone

Comments

@oliversiket
Copy link

Hello,

I encountered an issue with the following code:

$start = Carbon::parse('2021-03-31 23:00:00');

$start->month(4);

echo $start;

I reckon this should use noOverflow behind the scenes instead of overflow

Carbon version: v2.24.0

PHP version: 7.3.23

I expected to get:

2021-04-30 23:00:00

But I actually get:

2021-05-01 23:00:00

Thanks!

@kylekatarnls kylekatarnls added the duplicate Solution/explanation on this issue has already been given in an other issue label Apr 14, 2021
@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Apr 14, 2021

Hello 👋

This is a duplicate of #2309, #2260, #2176

This behavior is inherited from the native DateTime PHP class, please read the chapter about overflow in:
https://carbon.nesbot.com/docs/#api-addsub

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 $end->diffInMonths($start) >= 1, be aligned with DateTime objects, ensure the day won't be reduced, etc.

But I will soon work more actively on 3.x so there is room to reconsider this at this point.

@kylekatarnls kylekatarnls added this to the 3.0.0 milestone Apr 14, 2021
@kylekatarnls
Copy link
Collaborator

For the record, this also behave consistently with JavaScript Date object:

date = new Date('2021-01-31');
date.setMonth(1); // (index from January = 0)

console.log(date);

Output: Wed Mar 03 2021

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.

@kylekatarnls kylekatarnls modified the milestones: 3.0.0, Future Jan 24, 2024
@kylekatarnls
Copy link
Collaborator

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:

  • 2024-02-29
  • 2024-02-28 (because maybe leap year is an issue, for instance if the next operation is to change the year)
  • 2024-03-01 (overflow to next day if the 31 does not exist in the current month)
  • 2024-03-02 (or 03-03 on non-leap years, so the overflowing days are reported to next month)
  • 2024-02-31 (not possible out of the box with DateTime inherited class, but we may imagine a special invalid state for the object that would be able store an invalid date)

Then various possible expectations on the time part:

  • 23:00 (report time as is)
  • 23:59:59.999999 (in case we stay on February, we can consider take the latest moment of this month)
  • 00:00 (in case we overflow to the 1st, we can consider take the earliest moment of the next month)

So way too much combos, and the default one PHP and JS pick: 2024-03-02 23:00 is not that much worse than any other arbitrary position.

In early versions Carbon (and also PHP BTW) tended to throw as less errors as possible and just guess what the user may want. We progressively moved away for that for a more honest approach of throwing exception the user intent is ambiguous or can lead to unexpected results. So another output that has been considered for the code above would be to thrown an exception such as:

Unable to set month to February starting from 2024-03-31 because 2024-02-31 is not a valid date.

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: $date->settings(['overflow' => $option])->month(2); and $option could be some enum value to pick the behavior to overflow like DateTime does, to get last day of the month, or to throw an exception). But it introduces a lot of complexity to handle all the behaviors and the discoverability of the edge-cases remains.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate Solution/explanation on this issue has already been given in an other issue
Projects
None yet
Development

No branches or pull requests

2 participants