Skip to content

Enum error message consistency#9350

Merged
kocsismate merged 18 commits into
php:masterfrom
ollieread:enum-stringable-and-magic-methods
Aug 23, 2022
Merged

Enum error message consistency#9350
kocsismate merged 18 commits into
php:masterfrom
ollieread:enum-stringable-and-magic-methods

Conversation

@ollieread

@ollieread ollieread commented Aug 16, 2022

Copy link
Copy Markdown
Contributor

This PR addresses the points raised in #9262 by doing the following:

Updating magic method exception messages

Previously the error message:

Enum may not include magic method %s

Was only used for:

  • __sleep
  • __wakeup
  • __set_state

All of which I believe are "technically" not magic methods, whereas all of the other true magic methods gave the error message:

Enum may not include %s

This PR updates it so that all true magic methods also give the following error:

Enum may not include magic method %s

Normalising double quote usage in error messages

Enum error messages were inconsistent in their usage of double quotes with an enum name, as well as with values.

This PR changes the following error messages:

Properties

Enum "%s" may not include properties

Is now

Enum %s may not include properties

Backing

"%s" is not a valid backing value for enum "%s"

Is now

"%s" is not a valid backing value for enum %s

In the spirit of keeping this consistent, double quotes have been added to the value when the backing type is int.

Added missing tests

I've also added tests for all of the magic methods that weren't previously covered.

Stringable

Nothing was done with the Stringable interface, as originally planned, because the usage of __toString was always caught before the interface.

@iluuu1994 iluuu1994 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest looks like a clear improvement. 👍

Comment thread Zend/zend_enum.c Outdated
@iluuu1994

Copy link
Copy Markdown
Member

@ollieread Btw you can add --bless to the test parameters to automatically fix the test expectations. You might also want to ./configure with --enable-zend-test to run the internal enum tests.

@kocsismate kocsismate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest looks good, and I appreciate the effort to keep these messages consistent :)

Comment thread Zend/tests/enum/__clone.phpt Outdated
@@ -0,0 +1,15 @@
--TEST--
Enum __clone

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
Enum __clone
Enums cannot include the __clone magic method

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the tests could profit from this change

Comment thread Zend/tests/enum/__set.phpt Outdated
@ollieread

Copy link
Copy Markdown
Contributor Author

The rest looks good, and I appreciate the effort to keep these messages consistent :)

Thanks, and I'm happy to help. As an aside, if you know any other bits that could do with some improvement to consistency, I'm happy to look at them.

Comment thread Zend/tests/enum/__set.phpt Outdated
Comment thread Zend/zend_enum.c Outdated
Comment thread Zend/zend_compile.c Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants