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

PHP 7.4 Warning for string to number comparisons that may change #3917

Open
wants to merge 2 commits into
base: PHP-7.4
from

Conversation

1 participant
@nikic
Copy link
Member

nikic commented Mar 4, 2019

Companion to #3886 for RFC https://wiki.php.net/rfc/string_to_number_comparison. This implements a warning in PHP 7.4 that is thrown if the comparison result will be different in PHP 8 (if the change is accepted). For now this is just an unconditional warning and also prints the compared values.

nikic added some commits Mar 4, 2019

Don't throw a warning for 1/-1 flips on equality comparisons
While the comparison result changes, the equality comparison is not
going to care about it in the end.

@nikic nikic changed the title String comparison 7.4 PHP 7.4 Warning for string to number comparisons that may change Mar 4, 2019

@nikic nikic added the RFC label Mar 4, 2019

@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Mar 4, 2019

Results for running this on the Laravel test suite:

1) Illuminate\Tests\Database\DatabaseEloquentBelongsToTest::testIdsInEagerConstraintsCanBeZero
Result of comparison between 0 and "foreign.value" will change (0 to -1)

/home/nikic/laravel/src/Illuminate/Database/Eloquent/Relations/BelongsTo.php:140
/home/nikic/laravel/src/Illuminate/Database/Eloquent/Relations/BelongsTo.php:118
/home/nikic/laravel/tests/Database/DatabaseEloquentBelongsToTest.php:96

2) Illuminate\Tests\Support\SupportCollectionTest::testSearchReturnsFalseWhenItemIsNotFound
ErrorException: Result of comparison between 1 and "bar" will change (1 to -1)

/home/nikic/laravel/tests/Support/SupportCollectionTest.php:2113
/home/nikic/laravel/src/Illuminate/Support/Collection.php:1459
/home/nikic/laravel/tests/Support/SupportCollectionTest.php:2114
  1. is part of this sort() call: https://github.com/laravel/framework/blob/d0a33d4a398f2bd23c2689aac473a3453ece76cc/src/Illuminate/Database/Eloquent/Relations/BelongsTo.php#L140 I can't really tell whether or not the different sort order would be a problem.

  2. is due to https://github.com/laravel/framework/blob/d0a33d4a398f2bd23c2689aac473a3453ece76cc/tests/Support/SupportCollectionTest.php#L2113, which uses a $value < 1 && is_numeric($value) check. This warning is ultimately a false positive due to the is_numeric() check, though it would be better to do that check first.

@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Mar 4, 2019

Results for running this against pear-core tests (as a representative of a much older codebase):

/home/nikic/pear-core/tests/PEAR_Command_Install/install/test_bug18102.phpt:
001- tests done
001+ Warning: Result of comparison between 0 and "*" will change (0 to 1) in /home/nikic/pear-core/PEAR.php on line 545
002+ tests done

/home/nikic/pear-core/tests/PEAR_Command_Package/package-validate/test.phpt:
001- tests done
001+ Warning: Result of comparison between 255 and "package.xml" will change (1 to -1) in /home/nikic/pear-core/PEAR/PackageFile.php on line 318
002+ tests done
  1. is due to https://github.com/pear/pear-core/blob/2e88e8707cdf666eec647b1855d69bcd0ef75bd9/PEAR.php#L545. The 0 that is being compared against is most likely PEAR_INSTALLER_FAILED, which is an error code. This looks like a bug to me, because it ends up not testing the expected error code. This should use === "*".

  2. is due to https://github.com/pear/pear-core/blob/master/PEAR/PackageFile.php#L318, which does a strlen($file < 255) check. This is obviously a bug and should be strlen($file) < 255.

@nikic nikic referenced this pull request Mar 4, 2019

Merged

Comparison fixes #87

@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Mar 4, 2019

Results for running against symfony tests (I get many baseline errors, but this is the only new one):

4) Symfony\Component\Translation\Tests\IntervalTest::testTest with data set #0 (true, 3, '{1,2, 3 ,4}')
Result of comparison between 3 and " 3 " will change (0 to 1)

/home/nikic/symfony/src/Symfony/Component/Translation/Interval.php:61
/home/nikic/symfony/src/Symfony/Component/Translation/Tests/IntervalTest.php:27

This is because trailing whitespace is not allowed in numeric strings. Something that's the case right now, but we should change this prior to making these comparison semantics changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.