=> The RFC was withdraw due to many controversial points and overall rejection and won't be proposed again by the RFC author. The RFC author advises to not revive this RFC as it was already rejected. Even if you think you have a much better idea, think twice.
This RFC proposes to conditionally add a strict argument count check for function calls on PHP7, being sensitive towards implementations already depending on the variable-length argument lists API.
Strict checks on argument counts are good to catch bugs (sometimes dangerous bugs) on user space code. According to measurements presented on this RFC, done by running many FOOS projects test suites, the current PHP silent behavior is not helpful and we should fix it.
Even PHP internal functions have a strict argument count check:
strlen("foo", "bar"); // PHP warning: strlen() expects exactly 1 parameter, 2 given on line 1
This RFC proposes that the engine should emit a Warning when a function is called with an exceeding number of arguments:
/** * fn expects only one argument * @param $arg */ function fn($arg) {} fn(1); // Ok fn(1, 2); // Warning call_user_func_array("fn", [1, 2, 3]); // Warning // Warning: fn() expects at most 1 parameter, 2 given, defined in %s on line %u and called in %s on line %u // Warning: fn() expects at most 1 parameter, 3 given, defined in %s on line %u and called in %s on line %u
The implementation is aware of the native variable-length argument list API and won't emit any warning if the called function or method is explicitly variadic or implemented with:
func_get_arg();func_get_args();The following examples WON'T break:
/** fn expects a variable-length argument lists */ function fn($arg) { $arg = func_get_arg(0); $args = func_get_args(); } fn(1); // Ok fn(...[1, 2, 3, 4, 5]); // Ok call_user_func_array("fn", [1, 2, 3, 4, 5, 6, 7]); // Ok
The implementation also supports dynamic resolution of function names. So aliased calls to dynamic arguments API is also covered:
namespace some\namespace; use function func_get_arg as foo; use function func_get_args as bar; /** fn expects a variable-length argument lists */ function fn($arg) { $arg = foo(0); $args = bar(); } fn(1); fn(1, 2); // Ok call_user_func("fn", 1, 2, 3); // Ok
During compilation the function implementations will be verified as sensitive to dynamic argument calls.
In other words, all the functions and methods implemented with func_get_arg() and func_get_args()
will be marked with a ZEND_ACC_DYNAMIC_ARGCOUNT flag:
CG(active_op_array)->fn_flags |= ZEND_ACC_DYNAMIC_ARGCOUNT;
During script execution, when the function or method finally gets invoked with an exceeding argument count,
the engine will check for the flag and emit a warning in case current execute_data is not variadic
and was not marked as sensitive to dynamic argument count:
Warning: %s() expects at most %u parameters, %u given, defined in %s on line %u and called in %s on line %u
=> It's worth to mention that, currently, PHP implementation already checks for exceeding arguments on every function call, the proposal only adds a warning according to each function implementation. The performance impact is negligible.
Functions will be marked as sensitive to variable-length argument list even if the implementation contains dead code such as:
function fn($arg) { if(false){ func_get_args(); } } fn(1, 2, 3); // Ok
Maybe when other levels of optimizations get added to PHP and dead code gets wiped before execution this little limitation will vanish. But, right now, there is no advantages into checking for dead code and perhaps some might prefer the implementation this way.
The implementation is aware of the @ operator.
The patch won't affect method calls implemented with call. Missing methods have no signature thus there is no way to validate argument count.
and callStatic
Variadic functions are explicitly implemented to handle variable-length argument lists, and won't be affected by the patch.
This RFC considers that anonymous functions are not intended to have a formal signature and should skip the exceeding argument count check.
This RFC considers that functions and objects passed as callbacks (relates to invoke) should not be affected by the exceeding argument count check,
as well as dynamic function calls. So the following code is not affected:
$unknownCallable($arg, $arg, $arg); $someObj->$unknownDispatch($arg, $arg, $arg); $class::$unknownDispatch($arg, $arg, $arg);
This is a concession to provide a good alternative to people calling unknown functions and doing method|functional dispatching.
Current patch is not aware of “obfuscated” calls to func_get_arg() and func_get_args():
function fn() { $fn = "func_get_args" $fn(); } fn(1, 2, 3); // Warning: fn() expects at most 1 parameter, 3 given, defined in %s on line %d and called...
This is not a common use case. A github search for the term “func_get_args” haven't revealed a single result of the API being used in this way. Besides that, there is no legit reason to use the variable-length argument API like that.
Therefore, this RFC also proposes to warn when func_get_arg(), func_get_args() is called dynamically by emitting a Warning:
$fn = "func_get_args"; $fn(); Warning: func_get_args() should not be called dynamically...
=> This bit was not implemented on the patch yet. It will be implemented if we reach consensus on that or maybe after the voting phase if the RFC passes.
debug_backtrace()[0][“args”]; as a substitute for func_get_args() are candidates to be affected, but this is not guaranteed.
=> All localized BC breaks. Besides that, debug_backtrace() should not be abused this way in the first place, we have a dedicated API for that.
=> This is a good BC break. In a staggering majority of use cases, passing extra arguments is or will become a bug at some point.
=> I made a blog post to clarify possible misunderstandings of this RFC on community media, it contains an anecdotal user story you may find worth reading: Why strict argument count on function calls will make PHP better, saner, easier…
In this section we will explore how the BC breaks, claimed here to be isolated or easy to fix, really will affect package ecosystem out there.
Take this as a previously partially opinionated response to anyone saying that the BC breaks will be massive to the point of disproving the usefulness of what's being proposed by this RFC.
The proposed patch has been tested against many widely used packages. Some important packages displayed warnings regarding the strict argument count check. This is a list of the packages and how they were easily fixed.
This was the first package I had to test, for obvious reason. PHPUnit seems to be passing a wrong argument count in a single place and this produced a good amount of warnings while running tests suites, but all of them had the same common causes:
SebastianBergmann\Comparator\ScalarComparator::assertEquals() expects at most 5 parameters, 6 given, defined in %s/sebastian/comparator/src/ScalarComparator.php on line 89 and called in %s PHPUnit_Framework_MockObject_Generator::getObjectForTrait() expects at most 6 parameters, 7 given, defined in %s/phpunit/phpunit-mock-objects/src/Framework/MockObject$Generator.php on line 442 and called in %s
This is how the assertEquals method was defined, with 5 arguments:
function assertEquals($expected, $actual, $delta = 0.0, $canonicalize = false, $ignoreCase = false) {
This is how the method was called, with 6 arguments:
$comparator->assertEquals($value, $actual[$key], $delta, $canonicalize, $ignoreCase, $processed);
If you care to analyze the context, you will notice that the $processed argument is declared as a reference on the caller's scope and while being passed to the ScalarComparator::assertEquals method it's being completely ignored! This is probably not the intended behavior (possibly a bug). The other warning was also caused by a obvious mistake.
It would take less than 4 minutes to fix this and get PHPUnit ready for PHP7, in case the proposal is accepted. It's worth to say that PHPUnit has a big code base (counting all the dependencies) and only 2 warnings related to this RFC were emitted, according to the test suite coverage.
With such a big codebase, many warnings were expected. But no big issues found! Only a few warnings to fix:
Warning: Symfony\Component\Yaml\Parser::isStringUnIndentedCollectionItem() expects 0 parameters, 1 given, defined in vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php on line 668 and called in ~/.composer/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php on line 332
This is the method signature:
/** * Returns true if the string is un-indented collection item * * @return bool Returns true if the string is un-indented collection item, false otherwise */ private function isStringUnIndentedCollectionItem() { return (0 === strpos($this->currentLine, '- ')); }
This is how the method is being called all over the place inside the package:
$unindentedEmbedBlock = $this->isStringUnIndentedCollectionItem($this->currentLine);
Looks like some parameter was removed during some refactory but, since PHP doesn't warn about argument lists going out of bounds, the residual parameters were not removed. This one took me ~4 minutes to fix.
This caused many warnings on many other components test suites not listed here, but it was a one liner fix. The warning:
Symfony\Component\Process\Pipes\UnixPipes::getDescriptors() expects 0 parameters, 1 given, defined in %s/Symfony/Component/Process/Pipes/UnixPipes.php on line 53 and called on %s/Symfony/Component/Process/Process.php
This is how the method was defined, with no arguments:
/** * {@inheritdoc} */ public function getDescriptors() {
This is how the method was called, with one argument:
$descriptors = $this->processPipes->getDescriptors($this->outputDisabled);
Synfony has very high quality packages we all rely on a daily basis, but this clearly was a mistake that could have been avoided if PHP was helping instead of hiding the problems.
All other components were fine. A good amount of tests of many components failed or hanged forever for reasons unrelated to the proposed patch (most likely PHP7 bugs). Which means PHP7 has some incompatibilities that probably won't be so straightforward to fix, at least when compared to what's being proposed on this RFC.
Composer converts all errors to exceptions. That's understandable because composer is built to be a reliable package manager, hence the low tolerance for errors. When running composer update:
[ErrorException] Symfony\Component\Process\Pipes\UnixPipes::getDescriptors() expects 0 parameters, 1 given, defined in phar:///usr/local/bin/composer/vendor/symfony/process/Symfony/Component/Process/Pipes/UnixPipes.php on line 53 and called...
If you've been reading this RFC with attention you already know that the cause of this warning was already covered a few paragraphs above and it's a one liner fix on Symfony/Process component. So, no big deal with composer too.
A few warnings were noticed on Laravel/Cache and Laravel/Validate components. All easily fixable. For instance:
Warning: Illuminate\Cache\RedisStore::forever() expects at most 2 parameters, 3 given, defined in %s/Illuminate/Cache/RedisStore.php on line 106 and called in %s/laravel/tests/Cache/CacheFileStoreTest.php:74
This is how the method was implemented, with 2 arguments:
/** * Store an item in the cache indefinitely. * * @param string $key * @param mixed $value * @return void */ public function forever($key, $value) {
This is how the method was called on a test case, with 3 arguments:
$store->forever('foo', 'Hello World', 60);
Again, this was probably not intended and is most likely to be a mistake. This could become problematic if, in the future, a new argument gets introduced
on the related method since the test is already silently passing an extra int 60 value on the method call pointed.
=> The other components had no warnings.
I really tried to run the test suite but so many things unrelated to this proposal caused fatal errors:
PHP Fatal error: Redefinition of parameter $notUsed in %s/zf2/tests/ZendTest/Db/Sql/Platform/IbmDb2/SelectDecoratorTest.php on line 60
After fixing this error, other errors appeared. Hence there is no numbers for ZF2. And it's probably safe to say that the strict argument count check proposed on this RFC will be the least important problem during PHP7 migration.
Wordpress test suite is basically composed by integration tests and a single warning on the code base is usually multiplied by thousands. At first this might sound scary: running the Wordpress test suite resulted in 4.597 warnings directly related to the proposed RFC. But after some heuristics it was noticeable that the warnings had a common cause:
I parsed the output file from the test case to detect repeated warnings and the result was a derisive number of only 27 unique warnings. In practice it means that 27 warnings were found, not 4.597. You can see the condensed list of unique warnings below:
Warning: wp_maybe_load_widgets() expects 0 parameters, 1 given, defined in %s/wp-includes/functions.php on line 3227 and called in %s/wp-includes/plugin.php on line 496 Warning: wp_maybe_load_embeds() expects 0 parameters, 1 given, defined in %s/wp-includes/media.php on line 2288 and called in %s/wp-includes/plugin.php on line 496 Warning: sanitize_comment_cookies() expects 0 parameters, 1 given, defined in %s/wp-includes/comment.php on line 1168 and called in %s/wp-includes/plugin.php on line 496 Warning: preview_theme() expects 0 parameters, 1 given, defined in %s/wp-includes/theme.php on line 649 and called in %s/wp-includes/plugin.php on line 496 Warning: create_initial_post_types() expects 0 parameters, 1 given, defined in %s/wp-includes/post.php on line 19 and called in %s/wp-includes/plugin.php on line 496 Warning: create_initial_taxonomies() expects 0 parameters, 1 given, defined in %s/wp-includes/taxonomy.php on line 21 and called in %s/wp-includes/plugin.php on line 496 Warning: wp_widgets_init() expects 0 parameters, 1 given, defined in %s/wp-includes/default-widgets.php on line 1440 and called in %s/wp-includes/plugin.php on line 496 Warning: WP_Widget_Factory::_register_widgets() expects 0 parameters, 1 given, defined in %s/wp-includes/widgets.php on line 582 and called in %s/wp-includes/plugin.php on line 496 Warning: WP_Http_Streams::test() expects at most 1 parameter, 2 given, defined in %s/wp-includes/class-http.php on line 1245 and called in %s/wp-includes/class-http.php on line 322 Warning: _show_post_preview() expects 0 parameters, 1 given, defined in %s/wp-includes/revision.php on line 518 and called in %s/wp-includes/plugin.php on line 496 Warning: _custom_header_background_just_in_time() expects 0 parameters, 1 given, defined in %s/wp-includes/theme.php on line 1646 and called in %s/wp-includes/plugin.php on line 496 Warning: WP_Http_Curl::test() expects at most 1 parameter, 2 given, defined in %s/wp-includes/class-http.php on line 1624 and called in %s/wp-includes/class-http.php on line 322 Warning: wp_ob_end_flush_all() expects 0 parameters, 1 given, defined in %s/wp-includes/functions.php on line 3270 and called in %s/wp-includes/plugin.php on line 496 Warning: twentyfifteen_setup() expects 0 parameters, 1 given, defined in %s/wp-content/themes/twentyfifteen/functions.php on line 54 and called in %s/wp-includes/plugin.php on line 496 Warning: twentyfifteen_custom_header_setup() expects 0 parameters, 1 given, defined in %s/wp-content/themes/twentyfifteen/inc/custom-header.php on line 15 and called in %s/wp-includes/plugin.php on line 496 Warning: twentyfifteen_widgets_init() expects 0 parameters, 1 given, defined in %s/wp-content/themes/twentyfifteen/functions.php on line 131 and called in %s/wp-includes/plugin.php on line 496 Warning: smilies_init() expects 0 parameters, 1 given, defined in %s/wp-includes/functions.php on line 2937 and called in %s/wp-includes/plugin.php on line 496 Warning: wp_cron() expects 0 parameters, 1 given, defined in %s/wp-includes/cron.php on line 312 and called in %s/wp-includes/plugin.php on line 496 Warning: wp_schedule_update_checks() expects 0 parameters, 1 given, defined in %s/wp-includes/update.php on line 626 and called in %s/wp-includes/plugin.php on line 496 Warning: check_theme_switched() expects 0 parameters, 1 given, defined in %s/wp-includes/theme.php on line 1889 and called in %s/wp-includes/plugin.php on line 496 Warning: WP_Widget_Recent_Posts::flush_widget_cache() expects 0 parameters, 1 given, defined in %s/wp-includes/default-widgets.php on line 790 and called in %s/wp-includes/plugin.php on line 496 Warning: kses_init() expects 0 parameters, 1 given, defined in %s/wp-includes/kses.php on line 1449 and called in %s/wp-includes/plugin.php on line 496 Warning: WP_Widget_Recent_Comments::flush_widget_cache() expects 0 parameters, 1 given, defined in %s/wp-includes/default-widgets.php on line 849 and called in %s/wp-includes/plugin.php on line 496 Warning: __clear_multi_author_cache() expects 0 parameters, 1 given, defined in %s/wp-includes/author-template.php on line 451 and called in %s/wp-includes/plugin.php on line 496 Warning: wp_timezone_override_offset() expects 0 parameters, 1 given, defined in %s/wp-includes/functions.php on line 3914 and called in %s/wp-includes/plugin.php on line 213 Warning: twentyfifteen_category_transient_flusher() expects 0 parameters, 1 given, defined in %s/wp-content/themes/twentyfifteen/inc/template-tags.php on line 166 and called in %s/wp-includes/plugin.php on line 496 Warning: delete_get_calendar_cache() expects 0 parameters, 1 given, defined in %s/wp-includes/general-template.php on line 1758 and called in %s/wp-includes/plugin.php on line 496
After checking case by case among the 27 warnings found, the conclusion is that all of them are a result of human mistake from some code refactory that left function calls with residual parameters behind. This could be fixed in less than 1 hour.
=> You can see the raw Wordpress test output here.
During the tests it became clearly measurable that the proposed strict argument count check won't be an issue. Actually, it's quite the opposite. It will help to increase PHP code quality from PHP7 and forward as all warnings were useful to catch mistakes or even bugs.
=> Many other packages were tested and resulted in very similar results, and in the end the ones that presented more issues or unique properties were listed here. For instance, Composer was chosen because it converts all error levels to exceptions and Wordpress was chosen for having a massive procedural code base and popularity...
So far we've been listing the advantages of this RFC on cases that out of bounds argument lists are a result of user mistakes. According to tests, mistakes and bugs represent the unquestionable majority of the situations.
But there are people exploiting the current PHP silent behavior intentionally. This section explores the possible drawbacks towards this group of users and how the warning could be fixed.
Sometimes a package contains an interface that defines some method with a given number of arguments but later it's noticed that the implementations of that given interface could benefit from additional arguments. This usually means that the interface needs to be updated to fit the new needs but...
To avoid updating the interface and releasing another major version of a package, some maintainers have been adding extra optional arguments to the implementations itself. Like what happened with the Zend/Validator component.
This is how the interface looks like:
interface ValidatorInterface { public function isValid($value); //...
And this is how the interface was implemented sometimes. Notice the additional $context argument.:
class Csrf implements ValidatorInterface { public function isValid($value, $context = null) { //...
This has been considered an acceptable “PHPism” but would now require a better solution. This was one of the reaction towards the proposal on twitter that illustrates the situation pretty well:
Tweet: I'm torn. On the 1 hand, we have a valid use case. On another: we should update our interface.
This is perfectly fixable though. The options are:
=> As the author of the RFC I'd say it's not a bad thing. But in fairness to release managers of PHP packages out there, this should also be discussed.
This is proposed for the next PHP x, which at the time of this writing would be PHP 7.
Vote started on 15th March and ends on 29th March.
As this is a language change, the main voting requires a 2/3 majority to pass.
This is a secondary voting to decide what error level should be emitted: E_WARNING or E_NOTICE.
A 50% + 1 majority is required. In the event of a tie the RFC author will take the final decision.
=> Some unrelated tests broke because the new emitted warning evidenced bugs on them. These tests were fixed without prejudice to the test suite.
func_get_arg* won't fatal anymore, the proposal now generates a warning.E_DEPRECATE option was removed because it implies future changes and if somebody is willing to push for that, another RFC should be created.E_STRICT option was removed so we don't conflict conceptually with the Reclassify E_STRICT RFC.func_num_args() as it's is not directly associated with variadic behaviors