-
-
Notifications
You must be signed in to change notification settings - Fork 142
POC: verifying polyfill signature using better reflection #294
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
Conversation
72144d2
to
38354fe
Compare
example output:
|
25e8d9c
to
8eacb15
Compare
8eacb15
to
8e295b6
Compare
fun, CI is reporting 93 incorrect signatures, locally ( PHP 8 RC2 ), its reporting 101 😅 travis is probably using beta2 |
# - 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 🎉
$toString = function($parameters) { | ||
return implode(', ', array_map(function($parameter) { | ||
return '$'.$parameter; | ||
}, $parameters)); | ||
}; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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. |
Thanks for the idea! |
this is just a draft for now.
ref #292