fix: use brick/math and ramsey/uuid 4 #71
Conversation
|
ramsey/uuid is already being upgraded in #69 |
composer.json
Outdated
| "ramsey/uuid": "^3.0" | ||
| "php": "^7.2", | ||
| "brick/math": "^0.8", | ||
| "ramsey/uuid": "^4.0" |
There was a problem hiding this comment.
I don't think there's any reason to force downstream users to upgrade to ramsey/uuid 4, unless this library uses features only available in version 4, and I don't think it does.
|
I agree with @ramsey about forcing users to upgrade. However, to maintain the close compatibility, I would like to use this strategy:
This way, current @pmccarren With that in mind, could you rebase this PR with master (which already features the support for |
|
@pascaldevink absolutely, I'd be happy to. Stand by |
| @@ -11,12 +11,13 @@ | |||
| } | |||
| ], | |||
| "require": { | |||
| "php": "^7.1", | |||
| "php": "^7.2 || ^8", | |||
There was a problem hiding this comment.
This is bad idea, PHP 8 is not even close to getting an alpha release and more BC breaks are coming.
There was a problem hiding this comment.
I was on the fence myself. but this matches the requirements of ramsey/uuid which this package is based on, so it should be safe.
There was a problem hiding this comment.
The builds consistently fail for nightlies on Travis CI, but when I completely ignore platform requirements locally and tweak a few things (e.g., Reflection APIs), then everything runs smoothly on 8.0.0-dev. My hope is that, over time, more projects will start using || ^8 and addressing the issues in their libraries that break on PHP 8 (and are known to be stable right now, such as Reflection APIs). One day, it will be nice to see the nightlies magically start passing. 😉
There was a problem hiding this comment.
If anyone wants to juggle with PHP 8 before at least first beta is out they can use --ignore-platform-reqs made for that purpose (you'd run composer install --ignore-platform-reqs in CI against php-nightly). But allowing PHP 8 in stable packages at this point is really bad idea, you're claiming to be compatible with something that doesn't even exist yet (remember PHP 6 book? 😃).
There was a problem hiding this comment.
I don't understand why it's a problem in stable packages. No one should be running PHP 8 in production, so having || ^8 in composer.json shouldn't cause any problems.
PHP 6 was an entirely different situation.
| { | ||
| $output = ''; | ||
| while ($number->getValue() > 0) { | ||
| while ($number->compareTo(0) > 0) { |
|
|
||
| $output .= $this->alphabet[(int)$digit->getValue()]; | ||
| $number = BigInteger::of( | ||
| bcdiv( |
There was a problem hiding this comment.
This does not seem right to me. You could do:
$number = $number->dividedBy($this->alphabetLength, RoundingMode::DOWN);| ); | ||
|
|
||
| $digit = $previousNumber->mod($this->alphabetLength); | ||
| $output .= $this->alphabet[(int)$digit->toInt()]; |
There was a problem hiding this comment.
No need to cast to int as toInt already returns an int.
| { | ||
| $number = new BigNumber(0); | ||
| $number = BigInteger::of(0); |
There was a problem hiding this comment.
Just a tip, there is a BigInteger::zero() method for this purpose.
|
Thanks all! I know it was a long time coming, but I've taken the commits from this PR by @pmccarren, used the feedback by @Majkl578 @ramsey and @marmichalski, and packages up version 3.0 that is all about introducing php 8 compatibility, and dropping support for php 7.1, 7.2, ramsey/uuid 3.x, and moontoast/math |
Use brick/math and upgrade ramsey/uuid to new 4.x release.
closes #68
Note: php 7.1 support is dropped, following support of ramsey/uuid@4