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

Carbon setters order matters and returns wrong date #1

Closed
tobischulz opened this issue Aug 11, 2022 · 4 comments
Closed

Carbon setters order matters and returns wrong date #1

tobischulz opened this issue Aug 11, 2022 · 4 comments

Comments

@tobischulz
Copy link

Hello,

I encountered an issue with the following code:

/**
 * Given a date as source that will be manipulated with setters
 * to a specific target date. 
 * We want to have the 2022-08-31 date with 08:00:00 time.
 */
$startAt = Carbon::parse('2022-06-29 08:00:00.0');
$temp = $startAt->copy();

// expecting 2022-08-31 08:00:00
echo $temp->day(31)->month(8)->year(2022) . PHP_EOL; // returns 2022-08-01 08:00:00

// expecting 2022-08-31 08:00:00
echo $temp->year(2022)->month(8)->day(31) . PHP_EOL; // returns 2022-08-31 08:00:00


/**
 * After fixing that by order of setters the same happens with different target dates
 * We want to have the 2025-02-05 date with 08:00:00 time.
 */
// expecting 2025-02-05 08:00:00
echo $temp->year(2025)->month(2)->day(5) . PHP_EOL; // returns 2025-03-05 08:00:00

// expecting 2025-02-05 08:00:00
echo $temp->day(1)->year(2025)->month(2)->day(5) . PHP_EOL; // returns 2025-02-05 08:00:00

Carbon version: 2.61.0

PHP version: 8.1

I expected to get:

2022-08-31 08:00:00

But I actually get:

2022-08-01 08:00:00

I guess that belongs to mutations of days through months the day like 31 does not exist and the date jumps to the next possible day. Is there a best practice-way to make sure this never happens? So this might not be a "bug" but i want to make sure to get my target dates and sleep well 😅.

Thanks!

@kylekatarnls
Copy link
Contributor

Best practice if you have a full date is to use 1 method to make 1 mutation instead of 3 successive mutations which is a waste of resources anyway:

$startAt = Carbon::parse('2022-06-29 08:00:00.0');

echo $startAt->modify('2022-08-31'); // Tell exactly what you expect and get it

// If you have integers:
$startAt = Carbon::parse('2022-06-29 08:00:00.0');

echo $startAt->setDate(2022, 8, 31);

Those are actually native methods from PHP DateTime object.

While year(), month(), day() are Carbon helpers calling setDate() so you actually do:

$startAt = new DateTime('2022-06-29 08:00:00.0');
$startAt->setDate($startAt->format('Y'), $startAt->format('n'), 31); // Impossible date => overflow to 2022-07-01
$startAt->setDate($startAt->format('Y'), 8, $startAt->format('j'));
$startAt->setDate(2022, $startAt->format('n'), $startAt->format('j'));

@tobischulz
Copy link
Author

This is awesome and makes totally sense. Thank you very much!

@ckilb
Copy link

ckilb commented Jan 2, 2025

I’d like to raise the question of whether we should consider re-opening this.
Here’s another simple example that demonstrated the issue:

Carbon::setTestNow('2024-12-31'); // Let's pretend, today is December 31st
$date = Carbon::now()->year(2024)->month(4)->day(1); // Instantiate a new date object

var_dump($date->format('Y-m-d')); // Expected: 2024/04/01  --- Actual: 2024/05/01

While the actual output makes sense if you know what’s happening behind the scenes, it’s highly unintuitive at first glance.
This behavior has caused bugs for me, and even some of my very experienced colleagues have run into the same issue, also producing bugs. Bugs that are hard to find out because they might only appear for some dates.

I’m not sure how to address this, but I think it’s worth discussing. Backward compatibility might be an important concern, but in my opinion, something needs to be changed here.

One possible solution could be to throw an exception if year or month is set to a value that results in an invalid date for the current instance. For example, if the current date’s month has 31 days and you set it to a month with fewer than 31 days, instead of silently adjusting the date's day to the next month, an exception could be thrown to clearly indicate the issue.

The same issue arises not only for changing the month, but the year - because of leap years.

What are your thoughts?

@kylekatarnls
Copy link
Contributor

kylekatarnls commented Jan 2, 2025

Hello, Carbon is not a builder, so every time you call ->years(), ->month() etc. it's yet another atomic date operation that got applied.

And so, it's less than ideal to use those methods to actually build a date. I.E. don't do:

$date->year(2024)->month(4)->day(1);

Do:

$date->setDate(2024, 4, 1);

So it's only 1 mutation and no overflow problem.

BTW, if you don't need the current time, then you don't need to start from Carbon::now(), just do Carbon::parse('2024-04-01').

Many overflow problems that got reported here and there often have this sub-optimal patterns (i.e. start from now or an arbitrary date while actually not using anything from it, or doing in multiple steps an operation that can be done in one go).

About throwing an exception when overflowing, it cannot happen on 3.x, but that's definitely an option on the table for 4.x.

The main problem about it is the discoverability issue. I.E. I build my code, it works, I ship it, but then errors happens only on some days or in some combos of operations, and so possibly don't get noticed before they hit many real users. (depending how logging is done, it might even goes under radar).

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

No branches or pull requests

3 participants