Brought over desired Carbon PHP related methods into RhubarbDateTime#62
Brought over desired Carbon PHP related methods into RhubarbDateTime#62jjbutnotreally wants to merge 2 commits intomasterfrom
Conversation
|
Note for anyone reviewing, this will bump the minimum version of PHP to 7.0. |
|
I'm having second thoughts - originally I thought you were going to 'reimplement' the 2 or 3 methods you were using from Carbon (start of week/end of week I think it was) but now that you've integrated Carbon code itself in, that opens other questions - main one being is the Apache 2 license compatible with the MIT license. I think it is - but it does make me think that there are lots of other methods in Carbon that would be nice. I'm going to propose (@samnotsowise interested in your opinion here) that it would be a lofty goal to
We need to prove this is practical. I'd like to suggest that:
Thoughts? |
|
This was dangling in my head the time I was bringing over this functionality. I didn't want to reinvent the wheel but at the same time I didn't just want to copy all of Carbon's logic. Carbon itself has just had a big rewrite for Carbon 2 so now would be a good time to go down the deprecation route. For what it's worth, when I was bringing this over a lot of the functionality that RhubarbDateTime provides is matched by Carbon (as they both extend \DateTime underneath), so I don't think the refactor will be that huge of a job. @acuthbert do you want me to have a look and see how the unit tests hold up? |
|
I agree, it'd be nice to just deprecate the RhubarbDate(Time) classes and update all of their methods to use Carbon equivalents in those classes (so long as they are identical). @acuthbert php 7.0 is EOL at the start of December. In my opinion, environment upgrades like that to support software lifecycles is really an issue which the maintainers of the environment need to take care of, and should not hamper our ability to adopt modern technologies into the framework. |
@acuthbert I've bumped the PHP version in composer.json and as a result all the hashes have updated in composer.lock. Can you verify this is the correct approach?
I've brought over the set of methods that we were looking for, in addition to the methods relied upon. I've also updated the tests to reflect the bulk of the new functionality.