Skip to content

feat: add Time::addCalendarMonths() function #9528

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

Open
wants to merge 12 commits into
base: 4.7
Choose a base branch
from

Conversation

christianberkman
Copy link
Contributor

@christianberkman christianberkman commented Apr 17, 2025

The Time::addMonths() could return an undesired behaviour if a date in the next calandar month is desired. For example, Time(2025-01-31)->addMonths(1) results in 2025-03-03 while a date in february may be desired, or Time(2025-03-31)->addMonths(1) results in a date in May not in April.

The addCalendarMonths function returns a new date in the next calendar month.
New: Also implemented subCalendarMonths function to return a new date in a previous calendar month.

Of course, this only applies if the dev is looking for a date in the next calendar month (months are weird of course, it is 30, 31, 28, 29 days...? who knows).

If deemed worthy to include in 4.7 I will write test and documentation.

Comparison between addMonths() and addCalendarMonths()

Initial date: 2025-01-31
+---------+------------+-------------------+
| $months | addMonths  | addCalendarMonths |
+---------+------------+-------------------+
| 0       | 2025-01-31 | 2025-01-31        |
| 1       | 2025-03-03 | 2025-02-28        |
| 2       | 2025-03-31 | 2025-03-31        |
| 3       | 2025-05-01 | 2025-04-30        |
| 4       | 2025-05-31 | 2025-05-31        |
| 5       | 2025-07-01 | 2025-06-30        |
| 6       | 2025-07-31 | 2025-07-31        |
| 7       | 2025-08-31 | 2025-08-31        |
| 8       | 2025-10-01 | 2025-09-30        |
| 9       | 2025-10-31 | 2025-10-31        |
| 10      | 2025-12-01 | 2025-11-30        |
| 11      | 2025-12-31 | 2025-12-31        |
| 12      | 2026-01-31 | 2026-01-31        |
| 13      | 2026-03-03 | 2026-02-28        |
+---------+------------+-------------------+

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@michalsn
Copy link
Member

Sounds useful.

In addition to fixing PHPStan, we also need:

  • tests for this new method
  • updated user guide
  • changelog entry

@michalsn michalsn added enhancement PRs that improve existing functionalities tests needed Pull requests that need tests 4.7 docs needed Pull requests needing documentation write-ups and/or revisions. labels Apr 18, 2025
@christianberkman
Copy link
Contributor Author

christianberkman commented Apr 18, 2025

Happy to write tests/docs/changelog.

I had tried returntype Time brefore but this also returns an error. Functions like addMonth() do not have any return type specified. What is the correct type?

Is there a way to run PHPStan only on one file, or does this break the testing? composer phpstan:check system/I18n/TimeTrait.php does not return any errors.

@paulbalandan
Copy link
Member

For phpstan, you should run it on the whole codebase, not just the changed file.

For testing, you can just add your example as a data provider then test on them.

Initial date: 2025-01-31
+---------+------------+-------------------+
| $months | addMonths | addCalendarMonths |
+---------+------------+-------------------+
| 0 | 2025-01-31 | 2025-01-31 |
| 1 | 2025-03-03 | 2025-02-28 |
| 2 | 2025-03-31 | 2025-03-31 |
| 3 | 2025-05-01 | 2025-04-30 |
| 4 | 2025-05-31 | 2025-05-31 |
| 5 | 2025-07-01 | 2025-06-30 |
| 6 | 2025-07-31 | 2025-07-31 |
| 7 | 2025-08-31 | 2025-08-31 |
| 8 | 2025-10-01 | 2025-09-30 |
| 9 | 2025-10-31 | 2025-10-31 |
| 10 | 2025-12-01 | 2025-11-30 |
| 11 | 2025-12-31 | 2025-12-31 |
| 12 | 2026-01-31 | 2026-01-31 |
| 13 | 2026-03-03 | 2026-02-28 |
+---------+------------+-------------------+

@christianberkman
Copy link
Contributor Author

christianberkman commented Apr 21, 2025

For phpstan, you should run it on the whole codebase, not just the changed file.

For testing, you can just add your example as a data provider then test on them.

Thank you, I figured out I need to run on the whole code base. Error should be gone now.

I wrote tests similar to testCanAddMonths() etc. I used a date that with a result in February.

christianberkman and others added 3 commits April 21, 2025 17:17
Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
revert change to TimeTrait::setTimeNow()
@paulbalandan paulbalandan removed the tests needed Pull requests that need tests label Apr 21, 2025
@michalsn michalsn removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.7 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants