Skip to content

Minimal fix for PHP 8 #154

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

Merged
merged 1 commit into from
Apr 7, 2021
Merged

Minimal fix for PHP 8 #154

merged 1 commit into from
Apr 7, 2021

Conversation

remicollet
Copy link
Contributor

To avoid TypeError when overflow to float

Don't know if it worth to be fixed as deprecated component... BTW, minor patch

@WyriHaximus WyriHaximus added this to the v0.5.11 milestone Apr 7, 2021
@WyriHaximus WyriHaximus requested review from clue, WyriHaximus and jsor April 7, 2021 14:32
@WyriHaximus WyriHaximus added the bug label Apr 7, 2021
@WyriHaximus
Copy link
Member

@remicollet Thanks! Is the same bug also present in react/http? As far as I'm considered, it's deprecated but a small bug fix like this we can release. We just wouldn't communicate it

@remicollet
Copy link
Contributor Author

@WyriHaximus in react/http, another fix is already applied

               $this->chunkSize = @\hexdec($hexValue);
               if (!\is_int($this->chunkSize) || \dechex($this->chunkSize) !== $hexValue) {

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remicollet Thanks for the update to support PHP 8, changes LGTM! :shipit:

The change itself is indeed very similar to reactphp/http#391, so I agree it makes sense to merge as-is even if this component has been deprecated (#152 / #153).

@WyriHaximus
Copy link
Member

@remicollet Ok awesome!

@WyriHaximus WyriHaximus merged commit ffa0540 into reactphp:master Apr 7, 2021
@WyriHaximus
Copy link
Member

Just released v0.5.11

@remicollet remicollet deleted the patch-1 branch April 7, 2021 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants