Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Enum class implements \Serializable interface #39

Closed
wants to merge 1 commit into from

Conversation

skwi
Copy link

@skwi skwi commented Oct 27, 2017

Implementation of feature asked in issue #26

Only values of type string and int are serializable with this PR since they are the recommended type in the documentation (https://github.com/skwi/PhpEnums#usage)

This is quite a constraint so let me know if it should be avoided.

@ogizanagi
Copy link
Member

Hi @skwi ! Thanks for the PR.

Despite I approved the related issue, I've never implemented it myself because I had two concerns afterwards. I'm glad there is someone to open the discussion here:

  1. The package features a really simple way to compare enums as each enumerated value will always return the same instance...but this does not work with deserialization, so it's flawed already. I don't know any way to workaround this, even by controlling the serialization. At least this limitation must appear in the documentation if we promote a feature about serializing an enum.

  2. I never really had to care about serializing enum types using php natives but serialize/unserialize just work already fine to me without any need to get more control over it, isn't it? What would be the real benefit of using the interface? The enum value is the only thing that is part of the state, and it's unlikely (and unadvised) to have any other instance property due to the nature of enum types. So, no perf consideration is involved, right?

    However, we could benefit from the \JsonSerializable interface as an hint for integrations, like in logs with the Monolog NormalizerFormatter.

Only values of type string and int are serializable with this PR since they are the recommended type in the documentation (https://github.com/skwi/PhpEnums#usage)

Indeed, I even replaced explicitly the docblocks from mixed to int|string because it should be the only types used for an enumerated value. Actually, the enumerated value shouldn't matter in any way and you're not supposed to rely on: only the identifier is important. As in other languages with native enum types, I'd like to generate by default internal values so you just have to declare identifiers, but that's not really doable unfortunately. I recommend strings so raw values are still human readable, and integers are mainly here for flagged enums needs.

That's why I personally think mathematic constants aren't a good use case for an enumerated type: it's not really a finite set, just an arbitrary choice list of constants in your app. But if it's really something that make sense as an enum in your app, you may just add a method returning the corresponding math constant for an instance/identifier.

@ogizanagi
Copy link
Member

I'm gonna close as won't fix for now with the aforementioned explications. Feel free to re-open/comment later if you've got new arguments to express.

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.

None yet

2 participants