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

POC: verifying polyfill signature using better reflection #294

Closed
wants to merge 1 commit into from

Conversation

azjezz
Copy link
Contributor

@azjezz azjezz commented Oct 20, 2020

this is just a draft for now.

ref #292

@azjezz
Copy link
Contributor Author

azjezz commented Oct 20, 2020

example output:

❯ php --version
PHP 8.0.0RC2 (cli) (built: Oct 18 2020 12:10:31) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.0-dev, Copyright (c) Zend Technologies
    with Zend OPcache v8.0.0RC2, Copyright (c), by Zend Technologies
❯ php vendor/bin/simple-phpunit tests/Mbstring
PHP Warning:  Private methods cannot be final as they are never overridden by other classes in /home/azjezz/Projects/polyfill/vendor/bin/.phpunit/phpunit-6.5/src/Util/Configuration.php
 on line 176

Warning: Private methods cannot be final as they are never overridden by other classes in /home/azjezz/Projects/polyfill/vendor/bin/.phpunit/phpunit-6.5/src/Util/Configuration.php on l
ine 176
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing tests/Mbstring
E.EFFEFFF....FF.E....E................................WWWWWWWWWWW 65 / 88 ( 73%)
WWWWWWWWWWWWWWWWWWWWWWW                                           88 / 88 (100%)

Time: 237 ms, Memory: 16.00MB

There were 5 errors:

1) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testStubs
ValueError: mb_substitute_character(): Argument #1 ($substitute_character) must be "none", "long", "entity" or a valid codepoint

/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:31

2) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testDecodeNumericEntity
TypeError: mb_decode_numericentity(): Argument #1 ($string) must be of type string, stdClass given

/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:62

3) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testEncodeNumericEntity
TypeError: mb_encode_numericentity(): Argument #1 ($string) must be of type string, stdClass given

/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:123

4) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testStrSplit
ValueError: mb_str_split(): Argument #2 ($length) must be greater than 0

/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:330

5) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testLanguage
ValueError: mb_language(): Argument #1 ($language) must be a valid language, "ABC" given

/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:406

--

There were 34 warnings:

1) Warning
The polyfill for "mb_convert_encoding" has an incorrect signature.

Expected : $string, $to_encoding, $from_encoding
Actual   : $s, $to, $from

2) Warning
The polyfill for "mb_decode_mimeheader" has an incorrect signature.

Expected : $string
Actual   : $s

3) Warning
The polyfill for "mb_encode_mimeheader" has an incorrect signature.

Expected : $string, $charset, $transfer_encoding, $newline, $indent
Actual   : $s, $charset, $transferEnc, $lf, $indent

4) Warning
The polyfill for "mb_decode_numericentity" has an incorrect signature.

Expected : $string, $map, $encoding
Actual   : $s, $convmap, $enc

5) Warning
The polyfill for "mb_encode_numericentity" has an incorrect signature.

Expected : $string, $map, $encoding, $hex
Actual   : $s, $convmap, $enc, $is_hex

6) Warning
The polyfill for "mb_convert_case" has an incorrect signature.

Expected : $string, $mode, $encoding
Actual   : $s, $mode, $enc

7) Warning
The polyfill for "mb_internal_encoding" has an incorrect signature.

Expected : $encoding
Actual   : $enc

8) Warning
The polyfill for "mb_language" has an incorrect signature.

Expected : $language
Actual   : $lang

9) Warning
The polyfill for "mb_check_encoding" has an incorrect signature.

Expected : $value, $encoding
Actual   : $var, $encoding

10) Warning
The polyfill for "mb_detect_encoding" has an incorrect signature.

Expected : $string, $encodings, $strict
Actual   : $str, $encodingList, $strict

11) Warning
The polyfill for "mb_detect_order" has an incorrect signature.

Expected : $encoding
Actual   : $encodingList

12) Warning
The polyfill for "mb_parse_str" has an incorrect signature.

Expected : $string, $result
Actual   : $s, $result

13) Warning
The polyfill for "mb_strlen" has an incorrect signature.

Expected : $string, $encoding
Actual   : $s, $enc

14) Warning
The polyfill for "mb_strpos" has an incorrect signature.

Expected : $haystack, $needle, $offset, $encoding
Actual   : $s, $needle, $offset, $enc

15) Warning
The polyfill for "mb_strtolower" has an incorrect signature.

Expected : $string, $encoding
Actual   : $s, $enc

16) Warning
The polyfill for "mb_strtoupper" has an incorrect signature.

Expected : $string, $encoding
Actual   : $s, $enc

17) Warning
The polyfill for "mb_substitute_character" has an incorrect signature.

Expected : $substitute_character
Actual   : $char

18) Warning
The polyfill for "mb_substr" has an incorrect signature.

Expected : $string, $start, $length, $encoding
Actual   : $s, $start, $length, $enc

19) Warning
The polyfill for "mb_stripos" has an incorrect signature.

Expected : $haystack, $needle, $offset, $encoding
Actual   : $s, $needle, $offset, $enc

20) Warning
The polyfill for "mb_stristr" has an incorrect signature.

Expected : $haystack, $needle, $before_needle, $encoding
Actual   : $s, $needle, $part, $enc

21) Warning
The polyfill for "mb_strrchr" has an incorrect signature.

Expected : $haystack, $needle, $before_needle, $encoding
Actual   : $s, $needle, $part, $enc

22) Warning
The polyfill for "mb_strrichr" has an incorrect signature.

Expected : $haystack, $needle, $before_needle, $encoding
Actual   : $s, $needle, $part, $enc

23) Warning
The polyfill for "mb_strripos" has an incorrect signature.

Expected : $haystack, $needle, $offset, $encoding
Actual   : $s, $needle, $offset, $enc

24) Warning
The polyfill for "mb_strrpos" has an incorrect signature.

Expected : $haystack, $needle, $offset, $encoding
Actual   : $s, $needle, $offset, $enc

25) Warning
The polyfill for "mb_strstr" has an incorrect signature.

Expected : $haystack, $needle, $before_needle, $encoding
Actual   : $s, $needle, $part, $enc

26) Warning
The polyfill for "mb_http_output" has an incorrect signature.

Expected : $encoding
Actual   : $enc

27) Warning
The polyfill for "mb_strwidth" has an incorrect signature.

Expected : $string, $encoding
Actual   : $s, $enc

28) Warning
The polyfill for "mb_substr_count" has an incorrect signature.

Expected : $haystack, $needle, $encoding
Actual   : $haystack, $needle, $enc

29) Warning
The polyfill for "mb_output_handler" has an incorrect signature.

Expected : $string, $status
Actual   : $contents, $status

30) Warning
The polyfill for "mb_convert_variables" has an incorrect signature.

Expected : $to_encoding, $from_encoding, $var, $vars
Actual   : $toEncoding, $fromEncoding, $a, $b, $c, $d, $e, $f

31) Warning
The polyfill for "mb_ord" has an incorrect signature.

Expected : $string, $encoding
Actual   : $s, $enc

32) Warning
The polyfill for "mb_chr" has an incorrect signature.

Expected : $codepoint, $encoding
Actual   : $code, $enc

33) Warning
The polyfill for "mb_scrub" has an incorrect signature.

Expected : $string, $encoding
Actual   : $s, $enc

34) Warning
The polyfill for "mb_str_split" has an incorrect signature.

Expected : $string, $length, $encoding
Actual   : $string, $split_length, $encoding

--

There were 7 failures:

1) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testDecodeNumericEntityWarnsOnInvalidInputType
Failed asserting that exception of type "TypeError" matches expected exception "PHPUnit\Framework\Error\Warning". Message was: "mb_decode_numericentity(): Argument #1 ($string) must be
 of type string, stdClass given" at
/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:105
.

2) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testDecodeNumericEntityWarnsOnInvalidEncodingType
Failed asserting that exception of type "TypeError" matches expected exception "PHPUnit\Framework\Error\Warning". Message was: "mb_decode_numericentity(): Argument #3 ($encoding) must
be of type ?string, stdClass given" at
/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:114
.

3) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testEncodeNumericEntityWarnsOnInvalidInputType
Failed asserting that exception of type "TypeError" matches expected exception "PHPUnit\Framework\Error\Warning". Message was: "mb_encode_numericentity(): Argument #1 ($string) must be
 of type string, stdClass given" at
/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:161
.

4) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testEncodeNumericEntityWarnsOnInvalidEncodingType
Failed asserting that exception of type "TypeError" matches expected exception "PHPUnit\Framework\Error\Warning". Message was: "mb_encode_numericentity(): Argument #3 ($encoding) must
be of type ?string, stdClass given" at
/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:170
.

5) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testEncodeNumericEntityWarnsOnInvalidIsHexType
Failed asserting that exception of type "TypeError" matches expected exception "PHPUnit\Framework\Error\Warning". Message was: "mb_encode_numericentity(): Argument #4 ($hex) must be of
 type bool, stdClass given" at
/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:180
.

6) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testStrpos
Failed asserting that 0 is false.

/home/azjezz/Projects/polyfill/tests/Mbstring/MbstringTest.php:277

7) Symfony\Polyfill\Tests\Mbstring\MbstringTest::testStrposEmptyDelimiter
Failed asserting that exception of type "PHPUnit\Framework\Error\Warning" is thrown.

ERRORS!
Tests: 88, Assertions: 500, Errors: 5, Failures: 7, Warnings: 34.

~/Projects/polyfill verify-signature
❯

@azjezz azjezz force-pushed the verify-signature branch 2 times, most recently from 25e8d9c to 8eacb15 Compare October 20, 2020 02:04
@azjezz
Copy link
Contributor Author

azjezz commented Oct 20, 2020

fun, CI is reporting 93 incorrect signatures, locally ( PHP 8 RC2 ), its reporting 101 😅 travis is probably using beta2

Comment on lines +12 to +27
# - php: 5.3
# dist: precise
# - php: 5.4
# dist: trusty
# - php: 5.5
# dist: trusty
# - php: 5.6
# dist: trusty
# - php: 7.0
# dist: trusty
# - php: 7.1
# - php: 7.2
# - php: 7.3
# env: SYMFONY_PHPUNIT_VERSION=7.2
# - php: 7.4
# env: SYMFONY_PHPUNIT_VERSION=7.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

speeding up the build for this draft :)

@@ -47,6 +47,7 @@ install:
- if [[ $TRAVIS_BRANCH = master ]]; then export COMPOSER_ROOT_VERSION=dev-master; else export COMPOSER_ROOT_VERSION=$TRAVIS_BRANCH.x-dev; fi
- composer --prefer-source install
- if [[ $TRAVIS_PHP_VERSION = nightly ]]; then phpenv global 7.1; ./vendor/bin/simple-phpunit install; phpenv global nightly; fi
- if [[ $TRAVIS_PHP_VERSION = nightly ]]; then composer require roave/better-reflection --ignore-platform-reqs --dev; fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems to work as expected 🎉

Comment on lines +167 to +171
$toString = function($parameters) {
return implode(', ', array_map(function($parameter) {
return '$'.$parameter;
}, $parameters));
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

coverting to $foo, $bar, $bar for readability :)

$polyfillParametersString = $toString($polyfillParameters);
$originalParametersString = $toString($originalParameters);

return TestListener::warning(<<<FMT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is a warning the right thing here? or should it be an error/failure?

Copy link
Member

Choose a reason for hiding this comment

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

a failure would be the best

@azjezz
Copy link
Contributor Author

azjezz commented Oct 20, 2020

If we are okay with this, i'll add checks for class methods, and fix all parameter names ( merge #287 here ), so we can merge it, however, we would probably need to fix even more parameter names once PHP 8.0 is released, as the parameter name migration is still going on in php/php-src.

@nicolas-grekas
Copy link
Member

merge #287 here

don't: I'd prefer merging #287 separately, once you tell me all names are ok (which this PR will prove and enforce eventually.)

@nicolas-grekas nicolas-grekas changed the base branch from master to main October 20, 2020 10:35
@nicolas-grekas
Copy link
Member

Thanks for the idea!
I took over in #316, using var-dumper instead of better-reflection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants