Skip to content

Normalize substr() behavior #6182

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Normalize substr() behavior #6182

wants to merge 3 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Sep 22, 2020 •
edited
Loading

Change substr() to not treat out-of-bounds start offsets or lengths specially and always return a string. Under the premise that this function shouldn't throw for out-of-bounds values, the current behavior of returning false in some cases doesn't serve a purpose -- it only corrupts the return type by adding a |false union.

This makes substr() line up with the behavior of mb_substr()/mb_strcut().

@nikic nikic requested a review from kocsismate September 22, 2020 09:03
@nikic nikic force-pushed the substr-consistency branch from 77744ac to d7ebdce Compare September 22, 2020 09:51
@nikic

This comment has been minimized.

Sign in to view
@MCMic
Copy link
Contributor

MCMic commented Sep 22, 2020 •
edited
Loading

strpos always returns an int, it seems you meant substr in both title and description where you wrote strpos.
[EDIT] The commit message as well

@nikic nikic changed the title Normalize strpos() behavior Normalize substr() behavior Sep 22, 2020
@nikic nikic mentioned this pull request Sep 22, 2020
Closed
@kocsismate
Copy link
Member

I had a quick look in our code base as well as a few OS projects of mine, and none of the substr()usages assumed that the return value can be false. So this sample also seems to confirm that the false return value can be removed. :)

@nikic nikic force-pushed the substr-consistency branch from d7ebdce to b8f0a5a Compare September 23, 2020 09:01
@Girgias
Copy link
Member

Girgias commented Sep 23, 2020

Did you check the iconv_substr() function too?

@nikic
Copy link
Member Author

nikic commented Sep 23, 2020

@Girgias I have now. substr(), mb_substr(), iconv_substr() and grapheme_substr() should now all have the same behavior.

@Girgias Girgias mentioned this pull request Sep 23, 2020
Closed
4 tasks
@chschneider
Copy link
Contributor

I'm assuming the mentioning of strpos() in this pull request is a typo.
But: There is also the inconsistency with the empty needle for strpos, i.e.
strpos("a", ""); -> 0
vs.
grapheme_strpos("a", ""); -> ValueError

@nikic
Copy link
Member Author

nikic commented Sep 23, 2020

@chschneider php/php-tasks#20

@php-pulls php-pulls closed this in 13b791c Sep 25, 2020
@jrfnl
Copy link
Contributor

jrfnl commented Oct 2, 2020

This just broke PHP_CodeSniffer... I will submit a fix there, however, I would like to kindly request for this change to get a very clear changelog entry as it is an insidious change and currently not mentioned in the changelog at all.

https://3v4l.org/jRFZG

What broke was the "$text1" case from the above code-sample being checked against false, while it now returns an empty string.

jrfnl added a commit to jrfnl/PHP_CodeSniffer that referenced this pull request Oct 2, 2020
@jrfnl
PHP 8.0 compatibility: bug fix - ignore annotations are broken
028f868 
A very recent change in PHP 8.0 changes the possible return values of the `substr()` function from:

> Pre-PHP 8:
> Returns the extracted part of string; or FALSE on failure, or an empty string.

> PHP 8-RC1:
> Returns the extracted part of string; or an empty string.

This is an insidious change as basically all code (strict) checking the return value of `substr()` against `false` will now be broken.

Checking the return value with `empty()` will fix this in a cross-version compatible manner as it allows for both `false` as well as an empty string being returned.

This change broke the ignore annotations as implemented in PHPCS.

The existing unit tests for the ignore annotations cover this change.

Includes removing some unnecessary, duplicate function calls to `substr()`.

Ref: php/php-src#6182
@jrfnl jrfnl mentioned this pull request Oct 2, 2020
Merged
@nikic
Copy link
Member Author

nikic commented Oct 2, 2020

Thanks for the reminder, I've added an upgrading note in d7243ce.

@jrfnl
Copy link
Contributor

jrfnl commented Oct 2, 2020

Thanks @nikic !

jrfnl added a commit to jrfnl/PHP_CodeSniffer that referenced this pull request Oct 3, 2020
@jrfnl
PHP 8.0 compatibility: bug fix - ignore annotations are broken
3bc9ebf 
A very recent change in PHP 8.0 changes the possible return values of the `substr()` function from:

> Pre-PHP 8:
> Returns the extracted part of string; or FALSE on failure, or an empty string.

> PHP 8-RC1:
> Returns the extracted part of string; or an empty string.

This is an insidious change as basically all code (strict) checking the return value of `substr()` against `false` will now be broken.

Checking the return value with `empty()` will fix this in a cross-version compatible manner as it allows for both `false` as well as an empty string being returned.

This change broke the ignore annotations as implemented in PHPCS.

The existing unit tests for the ignore annotations cover this change.

Includes removing some unnecessary, duplicate function calls to `substr()`.

Ref: php/php-src#6182
jrfnl added a commit to jrfnl/PHP_CodeSniffer that referenced this pull request Oct 4, 2020
@jrfnl
PHP 8.0 compatibility: bug fix - ignore annotations are broken
4877700 
A very recent change in PHP 8.0 changes the possible return values of the `substr()` function from:

> Pre-PHP 8:
> Returns the extracted part of string; or FALSE on failure, or an empty string.

> PHP 8-RC1:
> Returns the extracted part of string; or an empty string.

This is an insidious change as basically all code (strict) checking the return value of `substr()` against `false` will now be broken.

Checking the return value with `empty()` will fix this in a cross-version compatible manner as it allows for both `false` as well as an empty string being returned.

This change broke the ignore annotations as implemented in PHPCS.

The existing unit tests for the ignore annotations cover this change.

Includes removing some unnecessary, duplicate function calls to `substr()`.

Ref: php/php-src#6182
gsherwood pushed a commit to squizlabs/PHP_CodeSniffer that referenced this pull request Oct 13, 2020
@jrfnl @gsherwood
PHP 8.0 compatibility: bug fix - ignore annotations are broken
1ea0f12 
A very recent change in PHP 8.0 changes the possible return values of the `substr()` function from:

> Pre-PHP 8:
> Returns the extracted part of string; or FALSE on failure, or an empty string.

> PHP 8-RC1:
> Returns the extracted part of string; or an empty string.

This is an insidious change as basically all code (strict) checking the return value of `substr()` against `false` will now be broken.

Checking the return value with `empty()` will fix this in a cross-version compatible manner as it allows for both `false` as well as an empty string being returned.

This change broke the ignore annotations as implemented in PHPCS.

The existing unit tests for the ignore annotations cover this change.

Includes removing some unnecessary, duplicate function calls to `substr()`.

Ref: php/php-src#6182
@Megatherium Megatherium mentioned this pull request Oct 19, 2020
Closed
@Findus23 Findus23 mentioned this pull request Nov 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nikic @MCMic @kocsismate @Girgias @chschneider @jrfnl