Only run validators once, remember the error

Author: DanielEScherzer

UPGRADING.INTERNALS

@@ -28,16 +28,11 @@ PHP 8.5 INTERNALS UPGRADE NOTES
     extra layer of indirection can be removed. In other cases a zval can
     be heap-allocated and stored in the pointer as a minimal change to keep
     compatibility.
-  * The validator callbacks for internal attribute now return `zend_string *`
+  . The validator callbacks for internal attribute now return `zend_string *`
     rather than `void`; instead of emitting an error when an attribute is
     applied incorrectly, the error message should be returned as a zend_string
-    pointer.
-  . The `target` parameter for validator callbacks for internal attributes
-    can now include in the bitmask the new flag
-    ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION. The flag indicates that
-    validation is being performed at runtime and that other than returning an
-    error message, the validator should not do anything, most notably including
-    modification of any class properties.
+    pointer. If the error will be delayed until runtime, it is stored in the
+    new `validation_error` field of the `zend_attribute` struct.
     RFC: https://wiki.php.net/rfc/delayedtargetvalidation_attribute
 
 - Hash

Zend/tests/attributes/delayed_target_validation/opcache_validator_errors.inc

@@ -0,0 +1,5 @@
+<?php
+
+#[DelayedTargetValidation]
+#[AllowDynamicProperties]
+trait DemoTrait {}

Zend/tests/attributes/delayed_target_validation/opcache_validator_errors.phpt

@@ -0,0 +1,58 @@
+--TEST--
+#[\DelayedTargetValidation] with preloaded attribute with errors
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+opcache.preload={PWD}/opcache_validator_errors.inc
+--EXTENSIONS--
+opcache
+--SKIPIF--
+<?php
+if (PHP_OS_FAMILY == 'Windows') die('skip Preloading is not supported on Windows');
+?>
+--FILE--
+<?php
+$r = new ReflectionClass('DemoTrait');
+echo $r . "\n";
+$attributes = $r->getAttributes();
+var_dump($attributes);
+try {
+	$attributes[1]->newInstance();
+} catch (Error $e) {
+	echo get_class($e) . ": " . $e->getMessage() . "\n";
+}
+
+?>
+--EXPECTF--
+Trait [ <user> trait DemoTrait ] {
+  @@ %s %d-%d
+
+  - Constants [0] {
+  }
+
+  - Static properties [0] {
+  }
+
+  - Static methods [0] {
+  }
+
+  - Properties [0] {
+  }
+
+  - Methods [0] {
+  }
+}
+
+array(2) {
+  [0]=>
+  object(ReflectionAttribute)#%d (1) {
+    ["name"]=>
+    string(23) "DelayedTargetValidation"
+  }
+  [1]=>
+  object(ReflectionAttribute)#%d (1) {
+    ["name"]=>
+    string(22) "AllowDynamicProperties"
+  }
+}
+Error: Cannot apply #[\AllowDynamicProperties] to trait DemoTrait

Zend/zend_attributes.c

@@ -89,11 +89,6 @@ static zend_string *validate_allow_dynamic_properties(
 		smart_str_append_printf(&str, msg, ZSTR_VAL(scope->name));
 		return smart_str_extract(&str);
 	}
-	if (target & ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION) {
-		// Should have passed the first time
-		ZEND_ASSERT((scope->ce_flags & ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES) != 0);
-		return NULL;
-	}
 	scope->ce_flags |= ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES;
 	return NULL;
 }
@@ -221,23 +216,15 @@ ZEND_METHOD(Deprecated, __construct)
 static zend_string *validate_nodiscard(
 	zend_attribute *attr, uint32_t target, zend_class_entry *scope)
 {
-	/* There isn't an easy way to identify the *method* that the attribute is
-	 * applied to in a manner that works during both compilation (normal
-	 * validation) and runtime (delayed validation). So, handle them separately. */
-	if (CG(in_compilation)) {
-		ZEND_ASSERT((target & ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION) == 0);
-		zend_op_array *op_array = CG(active_op_array);
-		const zend_string *prop_info_name = CG(context).active_property_info_name;
-		if (prop_info_name == NULL) {
-			op_array->fn_flags |= ZEND_ACC_NODISCARD;
-			return NULL;
-		}
-		// Applied to a hook, either throw or ignore
-		return ZSTR_INIT_LITERAL("#[\\NoDiscard] is not supported for property hooks", 0);
+	ZEND_ASSERT(CG(in_compilation));
+	zend_op_array *op_array = CG(active_op_array);
+	const zend_string *prop_info_name = CG(context).active_property_info_name;
+	if (prop_info_name == NULL) {
+		op_array->fn_flags |= ZEND_ACC_NODISCARD;
+		return NULL;
 	}
-	/* At runtime, no way to identify the target method; Reflection will handle
-	 * throwing the error if needed. */
-	 return NULL;
+	// Applied to a hook
+	return ZSTR_INIT_LITERAL("#[\\NoDiscard] is not supported for property hooks", 0);
 }
 
 ZEND_METHOD(NoDiscard, __construct)
@@ -467,6 +454,9 @@ static void attr_free(zval *v)
 
 	zend_string_release(attr->name);
 	zend_string_release(attr->lcname);
+	if (attr->validation_error != NULL) {
+		zend_string_release(attr->validation_error);
+	}
 
 	for (uint32_t i = 0; i < attr->argc; i++) {
 		if (attr->args[i].name) {
@@ -499,6 +489,7 @@ ZEND_API zend_attribute *zend_add_attribute(HashTable **attributes, zend_string
 	}
 
 	attr->lcname = zend_string_tolower_ex(attr->name, persistent);
+	attr->validation_error = NULL;
 	attr->flags = flags;
 	attr->lineno = lineno;
 	attr->offset = offset;

Zend/zend_attributes.h

@@ -34,10 +34,6 @@
 #define ZEND_ATTRIBUTE_IS_REPEATABLE		(1<<7)
 #define ZEND_ATTRIBUTE_FLAGS				((1<<8) - 1)
 
-/* Not a real flag, just passed to validators when target validation is
- * being run at runtime; must not conflict with any of the real flags above. */
-#define ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION (1<<8)
-
 /* Flags for zend_attribute.flags */
 #define ZEND_ATTRIBUTE_PERSISTENT   (1<<0)
 #define ZEND_ATTRIBUTE_STRICT_TYPES (1<<1)
@@ -64,6 +60,9 @@ typedef struct {
 typedef struct _zend_attribute {
 	zend_string *name;
 	zend_string *lcname;
+	/* Only non-null for internal attributes with validation errors that are
+	 * delayed until runtime via #[\DelayedTargetValidation] */
+	zend_string *validation_error;
 	uint32_t flags;
 	uint32_t lineno;
 	/* Parameter offsets start at 1, everything else uses 0. */

Zend/zend_compile.c

@@ -7606,8 +7606,10 @@ static void zend_compile_attributes(
 				if (error != NULL) {
 					if (delayed_target_validation == false) {
 						zend_error_noreturn(E_COMPILE_ERROR, "%s", ZSTR_VAL(error));
+						zend_string_efree(error);
+					} else {
+						attr->validation_error = error;
 					}
-					zend_string_efree(error);
 				}
 			}
 		} ZEND_HASH_FOREACH_END();

ext/opcache/zend_file_cache.c

@@ -461,6 +461,7 @@ static void zend_file_cache_serialize_attribute(zval                     *zv,
 
 	SERIALIZE_STR(attr->name);
 	SERIALIZE_STR(attr->lcname);
+	SERIALIZE_STR(attr->validation_error);
 
 	for (i = 0; i < attr->argc; i++) {
 		SERIALIZE_STR(attr->args[i].name);
@@ -1352,6 +1353,7 @@ static void zend_file_cache_unserialize_attribute(zval *zv, zend_persistent_scri
 
 	UNSERIALIZE_STR(attr->name);
 	UNSERIALIZE_STR(attr->lcname);
+	UNSERIALIZE_STR(attr->validation_error);
 
 	for (i = 0; i < attr->argc; i++) {
 		UNSERIALIZE_STR(attr->args[i].name);

ext/opcache/zend_persist_calc.c

@@ -181,6 +181,9 @@ static void zend_persist_attributes_calc(HashTable *attributes)
 			ADD_SIZE(ZEND_ATTRIBUTE_SIZE(attr->argc));
 			ADD_INTERNED_STRING(attr->name);
 			ADD_INTERNED_STRING(attr->lcname);
+			if (attr->validation_error != NULL) {
+				ADD_INTERNED_STRING(attr->validation_error);
+			}
 
 			for (i = 0; i < attr->argc; i++) {
 				if (attr->args[i].name) {

ext/reflection/php_reflection.c

@@ -7373,27 +7373,13 @@ ZEND_METHOD(ReflectionAttribute, newInstance)
 
 	/* Run the delayed validator function for internal attributes */
 	if (delayed_target_validation && ce->type == ZEND_INTERNAL_CLASS) {
-		zend_internal_attribute *config = zend_internal_attribute_get(attr->data->lcname);
-		if (config != NULL && config->validator != NULL) {
-			zend_string *error = config->validator(
-				attr->data,
-				attr->target | ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION,
-				attr->scope
-			);
-			if (error != NULL) {
-				zend_throw_exception(zend_ce_error, ZSTR_VAL(error), 0);
-				zend_string_efree(error);
-				RETURN_THROWS();
-			}
-		}
-		/* For #[NoDiscard], the attribute does not work on property hooks, but
-		 * at runtime the validator has no way to access the method that an
-		 * attribute is applied to, attr->scope is just the overall class entry
-		 * that the method is a part of. */
-		if (ce == zend_ce_nodiscard && attr->on_property_hook) {
-			zend_throw_error(NULL, "#[\\NoDiscard] is not supported for property hooks");
+		zend_string *error = attr->data->validation_error;
+		if (error != NULL) {
+			zend_throw_exception(zend_ce_error, ZSTR_VAL(error), 0);
 			RETURN_THROWS();
 		}
+	} else {
+		ZEND_ASSERT(attr->data->validation_error == NULL);
 	}
 
 	/* Repetition validation is done even if #[DelayedTargetValidation] is used