-
Notifications
You must be signed in to change notification settings - Fork 9
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
Upgrade Carbon to version 2 #38
Upgrade Carbon to version 2 #38
Conversation
Hi @OriHoch , This PR is failing against the nightly builds because the version specifier for PHPUnit v7 does not allow for PHP 8.0.0-dev. I can see only two options to handle this:
I have taken option 2 for now, and added PHP 7.3 to the list PHP versions in place of the nightly builds. |
Thanks a lot @courtney-miles, the PR looks good. I know that PHP < 7 is not supported anymore but I think it still has a lot of users. We would generally like the frictionless data libraries to support a large user-base. I opened a new issue for dropping PHP 5.6 support to try and get some comments on that: #39 |
Hi @OriHoch , I have realised that I believe I can preserve compatibility with PHP 5.6 with this PR. The existing tests only identified one BC breakage between Carbon v1 and v2, which I have worked around. So as long as we maintain compatibility with both Carbon v1 and v2, then I believe we can also maintain compatibility with PHP 5 and PHP 7. If you would prefer to see that PHP 5 compatibility is maintained, I would be happy to revise my changes to accommodate. Cheers |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Is there any interest from frictionlessdata for this PR? Whilst I appreciate wanting to keep this package open to those still using PHP 5, this will inevitably come at cost of losing users who keep their PHP version up to date, since there will be conflicts between the outdate packages this package requires and other packages that are keeping up to date with their dependencies. The versions already released will remain compatible with PHP 5, so those stuck on PHP 5 can still use this package. I can also make changes so this is compatible with both Carbon 1 and 2, which would allow you to keep compatability with PHP 5. Although this would not be my preference. If there's no interest, I'm happy to release and maintain my fork. But I don't want to do that unnecessarily. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
* Updated carbon dependency version. * Changes as per PR #38
This PR upgrades
nesbot/carbon
to version 2.This is prompted by a nag that carbon injects into composer output to inform that version 1 is no longer supported.
I have set PHP 7.1 as the new minimum version for this package because this is the minimum version supported by Carbon v2
Between Carbon v1 and v2, there was one compatibility breakage identified by the existing tests, which prompted the change to
DateField::validateCastValue()
. I believe, In version 1 Carbon::create() would raise an exception if supplied a non-integer for the$year
argument, where in version 2 is detects a string and uses it as a date string rather than a year number. So the logic changed to force validation for the "default" date format, rather than depend on Carbon to raise an exception.I have also raised the version of PHPUnit from version 4 to 7. No code changes were required.