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

Full support for PHP 8.1: Update preg_split() default $limit argument #101

Merged
merged 2 commits into from
Jan 26, 2022

Conversation

cdosoftei
Copy link
Contributor

This pull request addresses a deprecation concerning the $limit argument passed to preg_split():

Deprecated: preg_split(): Passing null to parameter #3 ($limit) of type int is deprecated in ./stdio-react/src/Readline.php on line 941

The adjustment should be compatible with the entire PHP version range supported by this library (>= 5.3).

@SimonFrings
Copy link
Contributor

@cdosoftei Thanks for looking into this one 👍

I guess you tested this with the PHP 8.1 version? If so, could you add 8.1 to the test matrix inside the .github\workflows\ci.yml. You also need to add convertDeprecationsToExceptions="true" to the phpunit.xml.dist then. For refs look into clue/ssh-proxy#32.

@cdosoftei
Copy link
Contributor Author

Thanks for the feedback @SimonFrings!

The requested updates are in.

@SimonFrings SimonFrings requested a review from clue January 26, 2022 11:37
@clue clue added this to the v2.6.0 milestone Jan 26, 2022
@clue clue changed the title Update preg_split() default $limit argument Full support for PHP 8.1: Update preg_split() default $limit argument Jan 26, 2022
Copy link
Owner

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

@cdosoftei Thanks for looking into this, changes LGTM! :shipit: Keep it up!

@clue clue merged commit 82d80ee into clue:main Jan 26, 2022
@spaceemotion
Copy link

I just stumbled upon this error and saw that it has been fixed already. Any idea when v2.6 comes out? I can't seem to force composer to install dev-main of this package instead...

@SimonFrings
Copy link
Contributor

Hey @spaceemotion, this will come out in the next couple of days, it's already on our ToDo list 👍

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.

4 participants