search

cakephp / chronos

A standalone DateTime library originally based off of Carbon
http://book.cakephp.org/chronos
MIT License
1.36k stars 63 forks source link

Fix off by one errors in diffFiltered #446

Closed markstory closed 11 months ago

markstory commented 11 months ago

Remove the work around that I added and add tests covering DifferenceTrait in Date classes. A similar fix will likely be needed in 3.x

othercorey commented 11 months ago

I'm not sure this fixes it. It's more like moving the plug from one leak to another. In 3.x, I tried to address it by allowing users to pass DatePeriod options into to the filtered wrappers.

The issue will still occur if a specific date range happens. If we were on PHP 8.2, I would have pushed to use https://www.php.net/manual/en/class.dateperiod.php#dateperiod.constants.include-end-date by default.

othercorey commented 11 months ago

See https://github.com/cakephp/chronos/pull/429

markstory commented 11 months ago

I'm not sure this fixes it. It's more like moving the plug from one leak to another

I don't disagree, but this change should fix the regression which is what I think 2.x users are looking for.

If we were on PHP 8.2, I would have pushed to use include-end-date

I agree this is the better solution as we can let users decide where their end date should be.

othercorey commented 11 months ago

Do you want to backport the 3.x changes with this?

markstory commented 11 months ago

Do you want to backport the 3.x changes with this?

I can take care of that.

markstory commented 11 months ago

Do you want to backport the 3.x changes with this?

@othercorey I had time to look at this today and I don't think that we'll be able to backport the additional option parameter as it would require changing ChronosInterface::diffFiltered which is a breaking change :cry:

othercorey commented 11 months ago

Ah ok. I had hoped it was easy, but users can just move to 3.x.