Enum magic method and interface inconsistencies

[ollieread]

Description

While researching within the PHP source codebase on Enums, I encountered several inconsistencies for which I've been unable to discover the reason.

Magic Method Errors

Enums cannot implement any magic methods, besides __call, __callStatic and __invoke. Implementing others will give you an E_COMPILE_ERROR of Enum may not include %s. But, because of how __sleep, __wakeup and __set_state are handled, implementing them gives you an E_COMPILE_ERROR of Enum may not include magic method %s. While I don't entirely understand why there's a different code path for those magic methods, I believe the difference is an internal implementation issue; therefore, the error messages for the end user should be consistent.

I'm pretty confident the solution for this would be to update the error message thrown by ZEND_ENUM_DISALLOW_MAGIC_METHOD, as the second error message that mentions magic methods is nicer and more descriptive. I do not know the impact of this, as I'm unsure where error messages fall when it comes to backwards compatibility.

I am happy to test and PR myself.

Banned Interfaces

Enums can implement any interface, except Serializable, which I suspect is down to the fact that this interface cannot be implemented without also implementing some of the magic methods that enum cannot have. There is a specific code path for this that throws an error.

I believe that the Stringable interface falls into this same category. Since this interface has a single method, __toString, it cannot be implemented without using one of the banned magic methods.

I think the best solution would be to add a specific check for Stringable within zend_verify_enum_interfaces. However, I'm unsure of the side effects as Stringable is both an implementable and an implicit interface.

I am happy to test and PR myself.

[iluuu1994]

Implementation-wise, anything that lives in the zend_class_entry is considered a magic-method, __sleep, __wakeup and __set_state are all just methods.

php-src/Zend/zend_enum.c

Lines 79 to 104 in 80197c5

	ZEND_ENUM_DISALLOW_MAGIC_METHOD(constructor, "__construct");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(destructor, "__destruct");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(clone, "__clone");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(__get, "__get");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(__set, "__set");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(__unset, "__unset");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(__isset, "__isset");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(__tostring, "__toString");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(__debugInfo, "__debugInfo");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(__serialize, "__serialize");
	ZEND_ENUM_DISALLOW_MAGIC_METHOD(__unserialize, "__unserialize");
	const char *forbidden_methods[] = {
	"__sleep",
	"__wakeup",
	"__set_state",
	};
	uint32_t forbidden_methods_length = sizeof(forbidden_methods) / sizeof(forbidden_methods[0]);
	for (uint32_t i = 0; i < forbidden_methods_length; ++i) {
	const char *forbidden_method = forbidden_methods[i];
	if (zend_hash_str_exists(&ce->function_table, forbidden_method, strlen(forbidden_method))) {
	zend_error_noreturn(E_COMPILE_ERROR, "Enum may not include magic method %s", forbidden_method);
	}
	}

However, they are all also listed in the documentation of magic methods so I agree that the error message should be the same. However, this is probably just an implementation oversight, since currently what are considered magic methods don't contain the "magic method" in the error message, but the ones that aren't do.

As for interfaces, we could also check if the enums implement a given interface to give a clearer error message, IMO it's not a big issue though.

[ollieread]

However, they are all also listed in the documentation of magic methods so I agree that the error message should be the same. However, this is probably just an implementation oversight, since currently what are considered magic methods don't contain the "magic method" in the error message, but the ones that aren't do.

As for interfaces, we could also check if the enums implement a given interface to give a clearer error message, IMO it's not a big issue though.

I agree that these aren't huge issues, as they don't really cause any problems except that they're slightly inconsistent. However, I'm happy to take the time to do this, as I think this could improve the language, in a minuscule way, but improve nonetheless.

[iluuu1994]

Sure, go ahead! Improvements are welcome of course.

[ollieread]

Implementation-wise, anything that lives in the zend_class_entry is considered a magic-method, __sleep, __wakeup and __set_state are all just methods.

Out of pure curiosity, is this something left-over from before magic methods, or is there an implementation issue that prevents anyone from making these methods truly magic methods?

[iluuu1994]

I'm not sure why __sleep and __wakeup are not proper magic methods. __set_state is special in that it's never actually called from the engine, it's only called from generated PHP code from var_export, so there's no point in storing it in zend_class_entry.

[ollieread]

Sure, go ahead! Improvements are welcome of course.

How do you want me to go about testing these @iluuu1994? The following magic methods are the only ones tested for:

• __call
• __callStatic
• __get
• __invoke
• __isset

Should I update __get and __isset because they'll now fail, and add one for each of the other methods that currently don't have a test?

[ollieread]

There are also inconsistencies with the exception messages and what should and should not be quoted.

• Enum may not include %s
• Enum "%s" may not include properties
• Enum may not include magic method %s
• Non-enum class %s cannot implement interface %s
• Enum case type %s does not match enum backing type %s
• Duplicate value in enum %s for cases %s and %s
• "%s" is not a valid backing value for enum "%s"

[ollieread]

@iluuu1994 just for clarification, what branch should I be targeting? I feel like this isn't a backwards compatibility issue, and it's not a new feature, though it's not a bug fix either.

[iluuu1994]

master should be fine as this is not a bug.

[ollieread]

I've updated the error messages and added tests for each of the magic methods @iluuu1994, but running any that test for errors gives me an exit code of 255 and appears just to be throwing an error, rather than testing for it. Any idea how to solve this?

/Users/ollieread/Code/php/php-src/sapi/cli/php
--TEST--
Enum __clone
--FILE--

Fatal error: Enum may not include magic method __clone in Standard input code on line 6

Process finished with exit code 255