From afc4d67c8b4e02a985a4cd27b8e79b343eb3c0ad Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Tue, 18 May 2021 12:28:20 +0200 Subject: [PATCH] Consistently treat optional-before-required as required There was a loophole here when it came to usage with named arguments, which was not intended. Close the loophole thoroughly by actually dropping the default value from the signature entirely. The default is still used to make the type nullable, but not for anything else. --- Zend/tests/call_user_func_005.phpt | 2 +- Zend/tests/required_param_after_optional.phpt | 6 ++-- ...uired_param_after_optional_named_args.phpt | 17 ++++++++++ Zend/zend_compile.c | 31 ++++++++++++++----- ext/reflection/tests/bug62715.phpt | 8 ++--- 5 files changed, 49 insertions(+), 15 deletions(-) create mode 100644 Zend/tests/required_param_after_optional_named_args.phpt diff --git a/Zend/tests/call_user_func_005.phpt b/Zend/tests/call_user_func_005.phpt index 2f5220db629..6a32f1970b5 100644 --- a/Zend/tests/call_user_func_005.phpt +++ b/Zend/tests/call_user_func_005.phpt @@ -18,7 +18,7 @@ var_dump(call_user_func(array('foo', 'teste'))); ?> --EXPECTF-- -Deprecated: Required parameter $b follows optional parameter $a in %s on line %d +Deprecated: Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter in %s on line %d string(1) "x" array(1) { [0]=> diff --git a/Zend/tests/required_param_after_optional.phpt b/Zend/tests/required_param_after_optional.phpt index cd715e77d48..93b98acfd9f 100644 --- a/Zend/tests/required_param_after_optional.phpt +++ b/Zend/tests/required_param_after_optional.phpt @@ -9,6 +9,8 @@ function test3(Type $test3A = null, Type2 $test3B = null, $test3C) {} ?> --EXPECTF-- -Deprecated: Required parameter $testC follows optional parameter $testA in %s on line %d +Deprecated: Optional parameter $testA declared before required parameter $testC is implicitly treated as a required parameter in %s on line %d -Deprecated: Required parameter $test2C follows optional parameter $test2B in %s on line %d +Deprecated: Optional parameter $testB declared before required parameter $testC is implicitly treated as a required parameter in %s on line %d + +Deprecated: Optional parameter $test2B declared before required parameter $test2C is implicitly treated as a required parameter in %s on line %d diff --git a/Zend/tests/required_param_after_optional_named_args.phpt b/Zend/tests/required_param_after_optional_named_args.phpt new file mode 100644 index 00000000000..e504d76c7ef --- /dev/null +++ b/Zend/tests/required_param_after_optional_named_args.phpt @@ -0,0 +1,17 @@ +--TEST-- +Optional param before required should be treated as required for named args as well +--FILE-- +getMessage(), "\n"; +} + +?> +--EXPECTF-- +Deprecated: Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter in %s on line %d +test(): Argument #1 ($a) not passed diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index fbb680be2f7..81c7009bda0 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -6466,7 +6466,6 @@ void zend_compile_params(zend_ast *ast, zend_ast *return_type_ast, uint32_t fall uint32_t i; zend_op_array *op_array = CG(active_op_array); zend_arg_info *arg_infos; - zend_string *optional_param = NULL; if (return_type_ast || fallback_return_type) { /* Use op_array->arg_info[-1] for return type */ @@ -6489,6 +6488,17 @@ void zend_compile_params(zend_ast *ast, zend_ast *return_type_ast, uint32_t fall arg_infos = safe_emalloc(sizeof(zend_arg_info), list->children, 0); } + /* Find last required parameter number for deprecation message. */ + uint32_t last_required_param = (uint32_t) -1; + for (i = 0; i < list->children; ++i) { + zend_ast *param_ast = list->child[i]; + zend_ast *default_ast_ptr = param_ast->child[2]; + bool is_variadic = (param_ast->attr & ZEND_PARAM_VARIADIC) != 0; + if (!default_ast_ptr && !is_variadic) { + last_required_param = i; + } + } + for (i = 0; i < list->children; ++i) { zend_ast *param_ast = list->child[i]; zend_ast *type_ast = param_ast->child[0]; @@ -6544,23 +6554,30 @@ void zend_compile_params(zend_ast *ast, zend_ast *return_type_ast, uint32_t fall zend_const_expr_to_zval(&default_node.u.constant, default_ast_ptr); CG(compiler_options) = cops; - if (!optional_param) { + if (last_required_param != (uint32_t) -1 && i < last_required_param) { /* Ignore parameters of the form "Type $param = null". * This is the PHP 5 style way of writing "?Type $param", so allow it for now. */ bool is_implicit_nullable = type_ast && Z_TYPE(default_node.u.constant) == IS_NULL; if (!is_implicit_nullable) { - optional_param = name; + zend_ast *required_param_ast = list->child[last_required_param]; + zend_error(E_DEPRECATED, + "Optional parameter $%s declared before required parameter $%s " + "is implicitly treated as a required parameter", + ZSTR_VAL(name), ZSTR_VAL(zend_ast_get_str(required_param_ast->child[1]))); } + + /* Regardless of whether we issue a deprecation, convert this parameter into + * a required parameter without a default value. This ensures that it cannot be + * used as an optional parameter even with named parameters. */ + opcode = ZEND_RECV; + default_node.op_type = IS_UNUSED; + zval_ptr_dtor(&default_node.u.constant); } } else { opcode = ZEND_RECV; default_node.op_type = IS_UNUSED; op_array->required_num_args = i + 1; - if (optional_param) { - zend_error(E_DEPRECATED, "Required parameter $%s follows optional parameter $%s", - ZSTR_VAL(name), ZSTR_VAL(optional_param)); - } } arg_info = &arg_infos[i]; diff --git a/ext/reflection/tests/bug62715.phpt b/ext/reflection/tests/bug62715.phpt index 63339cddc1e..b0080b6da63 100644 --- a/ext/reflection/tests/bug62715.phpt +++ b/ext/reflection/tests/bug62715.phpt @@ -17,9 +17,7 @@ foreach ($r->getParameters() as $p) { } ?> --EXPECTF-- -Deprecated: Required parameter $c follows optional parameter $b in %s on line %d -bool(true) -bool(true) +Deprecated: Optional parameter $b declared before required parameter $c is implicitly treated as a required parameter in %s on line %d +bool(false) +bool(false) bool(false) -NULL -int(0)