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 BaseDateRangeConditionRule not handling negative intervals correctly. #13463

Merged

Conversation

myleshyson
Copy link
Contributor

Description

This fixes a bug in the DateTimeHelper::toDateInterval method, where if passed a negative value, returns a positive interval. This caused an issue in DateRange::dateIntervalByTimePeriod, and subsequently the BaseDateRangeConditionRule.

I noticed this when trying to filter elements on the element index page by date period (e.g. After 1 day ago).

I went ahead and added a check for negative numbers in DateTimeHelper::toDateInterval and updated the DateTimeHelperTest to make sure it works. DateCreatedConditionRuleTest also passes now thanks to this fix.

Ran these tests using PHP version 8.2.4.

@myleshyson myleshyson requested a review from a team as a code owner July 21, 2023 18:33
@myleshyson
Copy link
Contributor Author

I also noticed some other tests failing when addressing this, like "MoneyTest" and "LanguageValidatorTest". I honestly didn't dig too deep into them. Maybe it's php 8.2 specific or I was missing something that testsuite needed.

@myleshyson myleshyson changed the title Fix BaseDateRangeConditionRule not handling negative interals correctly. Fix BaseDateRangeConditionRule not handling negative intervals correctly. Jul 21, 2023
@brandonkelly brandonkelly merged commit adb6234 into craftcms:develop Aug 2, 2023
@brandonkelly
Copy link
Member

Thank you!

I also noticed some other tests failing when addressing this, like "MoneyTest" and "LanguageValidatorTest".

Those are passing for me locally with this PR, so maybe a fluke.

brandonkelly added a commit that referenced this pull request Aug 2, 2023
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