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

Fix off by one errors in diffFiltered #446

Merged
merged 3 commits into from
Nov 3, 2023
Merged

Fix off by one errors in diffFiltered #446

merged 3 commits into from
Nov 3, 2023

Conversation

markstory
Copy link
Member

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

This captures the issue described in #445 as there are failing tests
for the same day scenario
Remove the workaround as it is causing #445.
@markstory markstory added this to the 2.x milestone Oct 29, 2023
@othercorey
Copy link
Member

othercorey commented Nov 1, 2023

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
Copy link
Member

See #429

@markstory
Copy link
Member Author

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
Copy link
Member

othercorey commented Nov 1, 2023

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

@othercorey othercorey merged commit 03208c1 into 2.x Nov 3, 2023
8 checks passed
@othercorey othercorey deleted the fix-445 branch November 3, 2023 05:26
@markstory
Copy link
Member Author

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

I can take care of that.

@markstory
Copy link
Member Author

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 😢

@othercorey
Copy link
Member

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

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.

2 participants