Conversation
5007e6a to
a17d348
Compare
Nyholm
left a comment
There was a problem hiding this comment.
It is a good thing you go straight to PHP 7.1
src/Hashids.php
Outdated
| $alphabet = $this->shuffle($alphabet, substr($lottery.$this->salt.$alphabet, 0, strlen($alphabet))); | ||
| $result = $this->unhash($subHash, $alphabet); | ||
| if ($this->math->greaterThan($result, PHP_INT_MAX)) { | ||
| if ($this->math->greaterThan($result, (string) PHP_INT_MAX)) { |
There was a problem hiding this comment.
This looks weird... why is this type cast needed?
There was a problem hiding this comment.
Yes, I agree. This is just me testing what works and not. This is still very much work in progress.
src/Hashids.php
Outdated
| * @return string | ||
| */ | ||
| protected function unhash($input, $alphabet) | ||
| protected function unhash(string $input, string $alphabet): string |
There was a problem hiding this comment.
Why should this return a string?
There was a problem hiding this comment.
It shouldn't, I'm just testing. Will fix in this in future commits.
3c6a349 to
4d07693
Compare
|
I realise this pull request has already been merged but I wanted to comment here just to make sure this mistake won't be made in the future. Just bumping requirements without actually implementing new features "just because PHP 5 will be EOL" doesn't add anything useful. Bumping requirements should be a natural process where you decide to use a new feature which in turn requires you to break compatibility. As long as there is no actual pressing need to do so, you shouldn't just bump requirements. The more compatible your code is, the better. I do realise you might want to incentivise your user base to upgrade to a newer version of PHP, but there are many cases where this simply might not be feasible, or necessary. (For example, PHP is used in internal systems that aren't web-facing and therefore are not susceptible to being hacked, while stability is more important than having the newest features. Also on embedded systems it's not always possible to update to the newest PHP versions.) So, hey, if you really want to bump to PHP 7.1 go ahead. But how about actually using some PHP 7.1 features like types while you're at it? |
We welcome any pull request you send our way. |
|
Bumping requirements without actually implementing new features is very irritating. I could waste my day fixing this, downgrade to 3.0.0, or just ditch the project altogether and write my own algo.
|
|
Exactly. It doesn’t even make sense. Why would you arbitrarily bump a requirement without using it? On the other side. This is why you should always pin composer requirements to specific major versions and never use “master” or “*”. |
This pull request removes support for legacy PHP versions. PHP 5.6 and 7.0 sees EOL this year and a lot of frameworks such as Laravel and Symfony has already dropped support for those versions.
This wont be an issue for users of older PHP versions since this will be a part of an major release and we won't be adding any breaking changes.