Error when attempting to upgrade to PHP8

[alexrivadeneira]

Hello,

We are attempting to upgrade our project to PHP8, and this error was surfaced by PHPCodesniffer and PHPCompatibility checker against PHP 8. Do you have any more information about this error?

FILE: .../vendor/hamcrest/hamcrest-php/tests/Hamcrest/Type/IsNumericTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 29 | ERROR | The behaviour of hexadecimal numeric strings was inconsistent
    |       | prior to PHP 7 and support has been removed in PHP 7. Found:
    |       | '0x4F2a04'
--------------------------------------------------------------------------------

[aik099]

That is a false-positive because the IsNumeric matcher is checking 2 things:

1. it's a string starting with 0x
2. the ctype_xdigit confirms, that it's a hexadecimal string

So no casting from hex to int happens.

P.S. I have no idea what PHP hexadecimal-related inconsistent behavior is checked by the PHPCompatibility coding standard. We do run tests on PHP8 as well.

[benlk]

The test seems to think that this use:

https://github.com/hamcrest/hamcrest-php/blob/8c3d0a3f6af734494ad8f6fbbee0ba92422859f3/tests/Hamcrest/Type/IsNumericTest.php#L28-L29

is a use of hex strings in a function which isn't known to support them:

https://github.com/PHPCompatibility/PHPCompatibility/blob/5c6bf16595c0f5ecd3df28f37d9aa7c4df66524c/PHPCompatibility/Sniffs/Numbers/RemovedHexadecimalNumericStringsSniff.php#L40-L102

The inconsistent behavior before PHP 7 is noted in the RFC that removed hex support: https://wiki.php.net/rfc/remove_hex_support_in_numeric_strings

This RFC proposes to remove support for hexadecimal numers in is_numeric_string. Support for hex in this function is inconsistent with behavior in other places - in particular PHP does not detect hex numbers when performing integer or float casts.

PHP internally has two primary methods from converting strings into numbers: <snip>

The first, and most commonly used, are direct casts to the integer or float types (convert_to_long and convert_to_double). These casts do NOT support hexadecimal numbers:

The second possibility is the is_numeric_string function, which will convert a string to either an integer or a float, whichever is more appropriate. This function does support hexadecimal numbers.

Just to cover edge cases, should the test case assert that the nonzero numbers given in this test suite are nonzero in addition to being numericValue(), and assert that the zero numbers are zero?

[aik099]

@benlk , As per my understanding the PHPCompatibility sniff reported a false-positive here because the code you've mentioned doesn't cause any test failures on any of the tested PHP versions (5.x, 7.x, 8.0).

What are you suggesting exactly?

[benlk]

As written, yes, this seems like a false positive, but it's alerted us to a deeper problem:

Using wp shell as a REPL under PHP 7.3:

wp> is_numeric( '0' )
=> bool(true)
wp> is_numeric( 0 )
=> bool(true)
wp> is_numeric( 0x4F2a04 )
=> bool(true)
wp> is_numeric( '0x4F2a04' )
=> bool(false)
> phpversion();
=> string(6) "7.3.32"

The assertion assertThat('0x4F2a04', numericValue()); should fail in PHP 7 and 8, since those versions of PHP do not treat such a string as numeric.

[aik099]

@benlk , looking at https://github.com/hamcrest/hamcrest-php/blob/master/hamcrest/Hamcrest/Type/IsNumeric.php#L30 method comment the code is expected to work like this: interpret hex numbers in any form (hex, string) as hex numbers and validate them.

[benlk]

Yes, that's what the method does. I'm calling into doubt whether that is the correct approach. Should this package reply "yes, it is numeric" if the language says "no, it is not numeric"? Who is the proper source of truth about the numeric-ness of the string? Should Hamcrest reflect what PHP says is true, or a language-agnostic truth?

[aik099]

This library attempts to mirror the original library version written in Java with some adaptations to PHP-specific stuff.

Not sure if the library should preserve BC of its method logic across all PHP versions or break it when PHP versions do.

I wish some of the other library maintainers could express some opinion on this topic.

[davedevelopment]

Morning! I would err on the side of caution here and maintain BC, but easy enough to raise a deprecation warning? It's hard to say what the users would expect here, if they're checking something is numeric for output, we're all good as we are, but if they are heading for casts or calculations, we need to potentially warn the user and they would need to make changes?