brick / date-time

Date and time library for PHP
MIT License
321 stars 29 forks source link

Fix Month enum argument deprecations within the library itself. #102

Closed gnutix closed 3 months ago

gnutix commented 3 months ago

Hello @BenMorel ,

I tried upgrading to 0.6.1 in our project, but it broke our Behat test suite because the library in itself is now triggering tons of deprecations via trigger_error(). Anyway, I tried to "finish the job" of using Month wherever there is a month concerned within the library itself, while leaving the usages of the |int alternative in the tests to make sure it still works.

I also had to use the Field\MonthOfYear::check($month); line before creating the Enum to keep the currently throwned DateTimeException, otherwise it would throw a PHP ValueError when given invalid values to BackendEnum::from().

Let me know what you think. I had to rush it quite a bit, so there's probably room for improvements. I'm not a fan of how the code looks right now, because of this compatibility layer, and even more because we don't store the Month instance internally, forcing to recreate it in a lot of places. But I'm also glad we don't change that, as already discussed. :sweat_smile:

BenMorel commented 3 months ago

@gnutix, I'm actually thinking of un-deprecating passing an int to these functions. After double-checking the original Java implementation, I noticed that they use method overloading to accept both int and Month whenever a month is accepted. A couple examples:

I feel like this gives greater flexibility to the library, WDYT? (You're still welcome to pass Month enums internally as per your PR though, if it makes the code more readable.)

Side note, I noticed that they strangely only accept int in withMonth() methods, I'm not sure why. But that shouldn't prevent us from accepting both.

Finally, could you please do one update per commit? Like:

  • Accept Month in LocalDate::of() & LocalDate::withMonth()
  • Accept Month in MonthDay::of() & MonthDay::withMonth()
  • ...
  • Convert internal calls to Month enum

This will make reviewing the code much easier. Thanks!

gnutix commented 3 months ago

Un-deprecating works for me. That will indeed give more flexibility to users, and it's not complex to maintain both input types.

I've split the changes into two PRs : removing deprecations and adding Month enum support :

PS: sorry for the spam, it took me a few iterations to get it right