Unify static/non-static check for magic methods

nikic · nikic · commit fbbcf82ab77c · 2020-07-20T10:39:43.000+02:00
And promote it to be fatal.
diff --git a/Zend/tests/bug61025.phpt b/Zend/tests/bug61025.phpt
@@ -3,10 +3,6 @@ Bug #61025 (__invoke() visibility not honored)
 --FILE--
 <?php
 
-Interface InvokeAble {
-    static function __invoke();
-}
-
 class Bar {
     private function __invoke() {
         return __CLASS__;
@@ -20,8 +16,6 @@ echo $b->__invoke();
 
 ?>
 --EXPECTF--
-Warning: The magic method InvokeAble::__invoke() cannot be static in %sbug61025.php on line %d
-
 Warning: The magic method Bar::__invoke() must have public visibility in %sbug61025.php on line %d
 Bar
 Fatal error: Uncaught Error: Call to private method Bar::__invoke() from global scope in %s:%d
diff --git a/Zend/tests/bug70215.phpt b/Zend/tests/bug70215.phpt
@@ -17,5 +17,4 @@ $b();
 
 ?>
 --EXPECTF--
-Warning: The magic method A::__invoke() cannot be static in %s on line %d
-A
+Fatal error: Method A::__invoke() cannot be static in %s on line %d
diff --git a/Zend/tests/errmsg_032.phpt b/Zend/tests/errmsg_032.phpt
@@ -12,4 +12,4 @@ class test {
 echo "Done\n";
 ?>
 --EXPECTF--
-Fatal error: Constructor test::__construct() cannot be static in %s on line %d
+Fatal error: Method test::__construct() cannot be static in %s on line %d
diff --git a/Zend/tests/errmsg_033.phpt b/Zend/tests/errmsg_033.phpt
@@ -12,4 +12,4 @@ class test {
 echo "Done\n";
 ?>
 --EXPECTF--
-Fatal error: Destructor test::__destruct() cannot be static in %s on line %d
+Fatal error: Method test::__destruct() cannot be static in %s on line %d
diff --git a/Zend/tests/errmsg_034.phpt b/Zend/tests/errmsg_034.phpt
@@ -12,4 +12,4 @@ class test {
 echo "Done\n";
 ?>
 --EXPECTF--
-Fatal error: Clone method test::__clone() cannot be static in %s on line %d
+Fatal error: Method test::__clone() cannot be static in %s on line %d
diff --git a/Zend/tests/magic_methods_003.phpt b/Zend/tests/magic_methods_003.phpt
@@ -11,4 +11,4 @@ class foo {
 
 ?>
 --EXPECTF--
-Warning: The magic method foo::__unset() cannot be static in %s on line %d
+Fatal error: Method foo::__unset() cannot be static in %s on line %d
diff --git a/Zend/tests/magic_methods_005.phpt b/Zend/tests/magic_methods_005.phpt
@@ -9,4 +9,4 @@ interface a {
 
 ?>
 --EXPECTF--
-Warning: The magic method a::__call() cannot be static in %s on line %d
+Fatal error: Method a::__call() cannot be static in %s on line %d
diff --git a/Zend/tests/magic_methods_006.phpt b/Zend/tests/magic_methods_006.phpt
@@ -9,4 +9,4 @@ interface a {
 
 ?>
 --EXPECTF--
-Warning: The magic method a::__callStatic() must be static in %s on line %d
+Fatal error: Method a::__callStatic() must be static in %s on line %d
diff --git a/Zend/tests/magic_methods_010.phpt b/Zend/tests/magic_methods_010.phpt
@@ -12,6 +12,4 @@ class a {
 --EXPECTF--
 Warning: The magic method a::__toString() must have public visibility in %s on line %d
 
-Warning: The magic method a::__toString() cannot be static in %s on line %d
-
 Fatal error: Method a::__toString() cannot take arguments in %s on line %d
diff --git a/Zend/tests/magic_methods_serialize.phpt b/Zend/tests/magic_methods_serialize.phpt
@@ -7,6 +7,4 @@ class Foo {
 }
 ?>
 --EXPECTF--
-Warning: The magic method Foo::__serialize() cannot be static in %s on line %d
-
 Fatal error: Method Foo::__serialize() cannot take arguments in %s on line %d
diff --git a/Zend/tests/magic_methods_set_state.phpt b/Zend/tests/magic_methods_set_state.phpt
@@ -11,4 +11,4 @@ class Foo {
 
 ?>
 --EXPECTF--
-Warning: The magic method Foo::__set_state() must be static in %s on line %d
+Fatal error: Method Foo::__set_state() must be static in %s on line %d
diff --git a/Zend/tests/magic_methods_unserialize.phpt b/Zend/tests/magic_methods_unserialize.phpt
@@ -7,6 +7,4 @@ class Foo {
 }
 ?>
 --EXPECTF--
-Warning: The magic method Foo::__unserialize() cannot be static in %s on line %d
-
 Fatal error: Method Foo::__unserialize() must take exactly 1 argument in %s on line %d
diff --git a/Zend/zend_API.c b/Zend/zend_API.c
@@ -2033,39 +2033,72 @@ static void zend_check_magic_method_args(
 	}
 }
 
+static void zend_check_magic_method_non_static(
+		const char *name, const zend_class_entry *ce, const zend_function *fptr, int error_type)
+{
+	if (fptr->common.fn_flags & ZEND_ACC_STATIC) {
+		zend_error(error_type, "Method %s::%s() cannot be static", ZSTR_VAL(ce->name), name);
+	}
+}
+
+static void zend_check_magic_method_static(
+		const char *name, const zend_class_entry *ce, const zend_function *fptr, int error_type)
+{
+	if (!(fptr->common.fn_flags & ZEND_ACC_STATIC)) {
+		zend_error(error_type, "Method %s::%s() must be static", ZSTR_VAL(ce->name), name);
+	}
+}
+
 ZEND_API void zend_check_magic_method_implementation(const zend_class_entry *ce, const zend_function *fptr, zend_string *lcname, int error_type) /* {{{ */
 {
 	if (ZSTR_VAL(fptr->common.function_name)[0] != '_'
 	 || ZSTR_VAL(fptr->common.function_name)[1] != '_') {
 		return;
 	}
 
-	if (zend_string_equals_literal(lcname, ZEND_DESTRUCTOR_FUNC_NAME)) {
+	if (zend_string_equals_literal(lcname, ZEND_CONSTRUCTOR_FUNC_NAME)) {
+		zend_check_magic_method_non_static("__construct", ce, fptr, error_type);
+	} else if (zend_string_equals_literal(lcname, ZEND_DESTRUCTOR_FUNC_NAME)) {
 		zend_check_magic_method_args(0, "__destruct", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__destruct", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, ZEND_CLONE_FUNC_NAME)) {
 		zend_check_magic_method_args(0, "__clone", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__clone", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, ZEND_GET_FUNC_NAME)) {
 		zend_check_magic_method_args(1, "__get", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__get", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, ZEND_SET_FUNC_NAME)) {
 		zend_check_magic_method_args(2, "__set", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__set", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, ZEND_UNSET_FUNC_NAME)) {
 		zend_check_magic_method_args(1, "__unset", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__unset", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, ZEND_ISSET_FUNC_NAME)) {
 		zend_check_magic_method_args(1, "__isset", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__isset", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, ZEND_CALL_FUNC_NAME)) {
 		zend_check_magic_method_args(2, "__call", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__call", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, ZEND_CALLSTATIC_FUNC_NAME)) {
 		zend_check_magic_method_args(2, "__callStatic", ce, fptr, error_type);
+		zend_check_magic_method_static("__callStatic", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, ZEND_TOSTRING_FUNC_NAME)) {
 		zend_check_magic_method_args(0, "__toString", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__toString", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, ZEND_DEBUGINFO_FUNC_NAME)) {
 		zend_check_magic_method_args(0, "__debugInfo", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__debugInfo", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, "__serialize")) {
 		zend_check_magic_method_args(0, "__serialize", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__serialize", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, "__unserialize")) {
 		zend_check_magic_method_args(1, "__unserialize", ce, fptr, error_type);
+		zend_check_magic_method_non_static("__unserialize", ce, fptr, error_type);
 	} else if (zend_string_equals_literal(lcname, "__set_state")) {
 		zend_check_magic_method_args(1, "__set_state", ce, fptr, error_type);
+		zend_check_magic_method_static("__set_state", ce, fptr, error_type);
+	} else if (zend_string_equals_literal(lcname, "__invoke")) {
+		zend_check_magic_method_non_static("__invoke", ce, fptr, error_type);
 	}
 }
 /* }}} */
@@ -2285,7 +2318,7 @@ ZEND_API int zend_register_functions(zend_class_entry *scope, const zend_functio
 			}
 			if (reg_function) {
 				zend_check_magic_method_implementation(
-					scope, reg_function, lowercase_name, error_type);
+					scope, reg_function, lowercase_name, E_CORE_ERROR);
 			}
 		}
 		ptr++;
@@ -2325,62 +2358,7 @@ ZEND_API int zend_register_functions(zend_class_entry *scope, const zend_functio
 		scope->__unserialize = __unserialize;
 		if (ctor) {
 			ctor->common.fn_flags |= ZEND_ACC_CTOR;
-			if (ctor->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "Constructor %s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(ctor->common.function_name));
-			}
-		}
-		if (dtor) {
-			if (dtor->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "Destructor %s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(dtor->common.function_name));
-			}
-		}
-		if (clone) {
-			if (clone->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "%s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(clone->common.function_name));
-			}
-		}
-		if (__call) {
-			if (__call->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "Method %s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(__call->common.function_name));
-			}
 		}
-		if (__callstatic) {
-			if (!(__callstatic->common.fn_flags & ZEND_ACC_STATIC)) {
-				zend_error(error_type, "Method %s::%s() must be static", ZSTR_VAL(scope->name), ZSTR_VAL(__callstatic->common.function_name));
-			}
-			__callstatic->common.fn_flags |= ZEND_ACC_STATIC;
-		}
-		if (__tostring) {
-			if (__tostring->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "Method %s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(__tostring->common.function_name));
-			}
-		}
-		if (__get) {
-			if (__get->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "Method %s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(__get->common.function_name));
-			}
-		}
-		if (__set) {
-			if (__set->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "Method %s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(__set->common.function_name));
-			}
-		}
-		if (__unset) {
-			if (__unset->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "Method %s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(__unset->common.function_name));
-			}
-		}
-		if (__isset) {
-			if (__isset->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "Method %s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(__isset->common.function_name));
-			}
-		}
-		if (__debugInfo) {
-			if (__debugInfo->common.fn_flags & ZEND_ACC_STATIC) {
-				zend_error(error_type, "Method %s::%s() cannot be static", ZSTR_VAL(scope->name), ZSTR_VAL(__debugInfo->common.function_name));
-			}
-		}
-
 		if (ctor && (ctor->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE)) {
 			zend_error_noreturn(E_CORE_ERROR, "Constructor %s::%s() cannot declare a return type", ZSTR_VAL(scope->name), ZSTR_VAL(ctor->common.function_name));
 		}
diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
@@ -6443,25 +6443,13 @@ static void zend_compile_implicit_closure_uses(closure_info *info)
 	ZEND_HASH_FOREACH_END();
 }
 
-static void zend_check_magic_method_attr(uint32_t attr, zend_class_entry *ce, const char* method, zend_bool is_static) /* {{{ */
+static void zend_check_magic_method_attr(uint32_t attr, zend_class_entry *ce, const char* method) /* {{{ */
 {
 	if (!(attr & ZEND_ACC_PUBLIC)) {
 		zend_error(E_WARNING,
 				"The magic method %s::%s() must have public visibility",
 				ZSTR_VAL(ce->name), method);
 	}
-
-	if (is_static) {
-		if (!(attr & ZEND_ACC_STATIC)) {
-			zend_error(E_WARNING,
-				"The magic method %s::%s() must be static",
-				ZSTR_VAL(ce->name), method);
-		}
-	} else if (attr & ZEND_ACC_STATIC) {
-		zend_error(E_WARNING,
-				"The magic method %s::%s() cannot be static",
-				ZSTR_VAL(ce->name), method);
-	}
 }
 /* }}} */
 
@@ -6540,44 +6528,44 @@ zend_string *zend_begin_method_decl(zend_op_array *op_array, zend_string *name,
 	} else if (zend_string_equals_literal(lcname, ZEND_CLONE_FUNC_NAME)) {
 		ce->clone = (zend_function *) op_array;
 	} else if (zend_string_equals_literal(lcname, ZEND_CALL_FUNC_NAME)) {
-		zend_check_magic_method_attr(fn_flags, ce, "__call", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__call");
 		ce->__call = (zend_function *) op_array;
 	} else if (zend_string_equals_literal(lcname, ZEND_CALLSTATIC_FUNC_NAME)) {
-		zend_check_magic_method_attr(fn_flags, ce, "__callStatic", 1);
+		zend_check_magic_method_attr(fn_flags, ce, "__callStatic");
 		ce->__callstatic = (zend_function *) op_array;
 	} else if (zend_string_equals_literal(lcname, ZEND_GET_FUNC_NAME)) {
-		zend_check_magic_method_attr(fn_flags, ce, "__get", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__get");
 		ce->__get = (zend_function *) op_array;
 		ce->ce_flags |= ZEND_ACC_USE_GUARDS;
 	} else if (zend_string_equals_literal(lcname, ZEND_SET_FUNC_NAME)) {
-		zend_check_magic_method_attr(fn_flags, ce, "__set", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__set");
 		ce->__set = (zend_function *) op_array;
 		ce->ce_flags |= ZEND_ACC_USE_GUARDS;
 	} else if (zend_string_equals_literal(lcname, ZEND_UNSET_FUNC_NAME)) {
-		zend_check_magic_method_attr(fn_flags, ce, "__unset", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__unset");
 		ce->__unset = (zend_function *) op_array;
 		ce->ce_flags |= ZEND_ACC_USE_GUARDS;
 	} else if (zend_string_equals_literal(lcname, ZEND_ISSET_FUNC_NAME)) {
-		zend_check_magic_method_attr(fn_flags, ce, "__isset", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__isset");
 		ce->__isset = (zend_function *) op_array;
 		ce->ce_flags |= ZEND_ACC_USE_GUARDS;
 	} else if (zend_string_equals_literal(lcname, ZEND_TOSTRING_FUNC_NAME)) {
-		zend_check_magic_method_attr(fn_flags, ce, "__toString", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__toString");
 		ce->__tostring = (zend_function *) op_array;
 		add_stringable_interface(ce);
 	} else if (zend_string_equals_literal(lcname, ZEND_INVOKE_FUNC_NAME)) {
-		zend_check_magic_method_attr(fn_flags, ce, "__invoke", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__invoke");
 	} else if (zend_string_equals_literal(lcname, ZEND_DEBUGINFO_FUNC_NAME)) {
-		zend_check_magic_method_attr(fn_flags, ce, "__debugInfo", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__debugInfo");
 		ce->__debugInfo = (zend_function *) op_array;
 	} else if (zend_string_equals_literal(lcname, "__serialize")) {
-		zend_check_magic_method_attr(fn_flags, ce, "__serialize", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__serialize");
 		ce->__serialize = (zend_function *) op_array;
 	} else if (zend_string_equals_literal(lcname, "__unserialize")) {
-		zend_check_magic_method_attr(fn_flags, ce, "__unserialize", 0);
+		zend_check_magic_method_attr(fn_flags, ce, "__unserialize");
 		ce->__unserialize = (zend_function *) op_array;
 	} else if (zend_string_equals_literal(lcname, "__set_state")) {
-        zend_check_magic_method_attr(fn_flags, ce, "__set_state", 1);
+        zend_check_magic_method_attr(fn_flags, ce, "__set_state");
 	}
 
 	return lcname;
@@ -7173,31 +7161,21 @@ void zend_compile_class_decl(znode *result, zend_ast *ast, zend_bool toplevel) /
 
 	if (ce->constructor) {
 		ce->constructor->common.fn_flags |= ZEND_ACC_CTOR;
-		if (ce->constructor->common.fn_flags & ZEND_ACC_STATIC) {
-			zend_error_noreturn(E_COMPILE_ERROR, "Constructor %s::%s() cannot be static",
-				ZSTR_VAL(ce->name), ZSTR_VAL(ce->constructor->common.function_name));
-		}
 		if (ce->constructor->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
 			zend_error_noreturn(E_COMPILE_ERROR,
 				"Constructor %s::%s() cannot declare a return type",
 				ZSTR_VAL(ce->name), ZSTR_VAL(ce->constructor->common.function_name));
 		}
 	}
 	if (ce->destructor) {
-		if (ce->destructor->common.fn_flags & ZEND_ACC_STATIC) {
-			zend_error_noreturn(E_COMPILE_ERROR, "Destructor %s::%s() cannot be static",
-				ZSTR_VAL(ce->name), ZSTR_VAL(ce->destructor->common.function_name));
-		} else if (ce->destructor->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
+		if (ce->destructor->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
 			zend_error_noreturn(E_COMPILE_ERROR,
 				"Destructor %s::%s() cannot declare a return type",
 				ZSTR_VAL(ce->name), ZSTR_VAL(ce->destructor->common.function_name));
 		}
 	}
 	if (ce->clone) {
-		if (ce->clone->common.fn_flags & ZEND_ACC_STATIC) {
-			zend_error_noreturn(E_COMPILE_ERROR, "Clone method %s::%s() cannot be static",
-				ZSTR_VAL(ce->name), ZSTR_VAL(ce->clone->common.function_name));
-		} else if (ce->clone->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
+		if (ce->clone->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
 			zend_error_noreturn(E_COMPILE_ERROR,
 				"Clone method %s::%s() cannot declare a return type",
 				ZSTR_VAL(ce->name), ZSTR_VAL(ce->clone->common.function_name));
diff --git a/tests/classes/__call_007.phpt b/tests/classes/__call_007.phpt

Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,4 @@ class Foo {`
`7`	`7`	`}`
`8`	`8`	`?>`
`9`	`9`	`--EXPECTF--`
`10`		`-Warning: The magic method Foo::__serialize() cannot be static in %s on line %d`
`11`		`-`
`12`	`10`	`Fatal error: Method Foo::__serialize() cannot take arguments in %s on line %d`