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

Brought over desired Carbon PHP related methods into RhubarbDateTime #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jjbutnotreally
Copy link

@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.

@jjbutnotreally
Copy link
Author

Note for anyone reviewing, this will bump the minimum version of PHP to 7.0.

@acuthbert
Copy link
Contributor

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

  1. Deprecate the rhubarb date classes
  2. Make other canonical modules in Rhubarb use Carbon
  3. Make RhubarbDateTime extend Carbon for backward compatibility

We need to prove this is practical. I'd like to suggest that:

  1. We see how the unit tests for RhubarbDateTime and more importantly RhubarbDate hold up if we just change what they extend to Carbon. If they all pass - brill. If not, how bad is it?
  2. We understand why Carbon currently requires php 7.1 - how aggressive are they with adopting the latest PHP stuff (Greenbox for example is on PHP 7.0 - so wouldn't be able to use Rhubarb if it brought Carbon in on the current version)

Thoughts?

@jjbutnotreally
Copy link
Author

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?

@samnotsowise
Copy link
Contributor

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.

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.

3 participants