Consistently treat optional-before-required as required

nikic · nikic · commit afc4d67c8b4e · 2021-05-18T12:40:58.000+02:00
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.
diff --git 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
@@ -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
@@ -0,0 +1,17 @@
+--TEST--
+Optional param before required should be treated as required for named args as well
+--FILE--
+<?php
+
+function test($a = 1, $b) {
+}
+try {
+    test(b: 2);
+} catch (Error $e) {
+    echo $e->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
@@ -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
@@ -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)
