unserialize can only work once

[delbertooo]

The so called "magic" from EnumSerializableTrait can only work once. If unserialize is called more than one time, it can't be compared by reference anymore.

This isn't your fault at all. It's just not possible to build this in php right now.

IMO the serializable support should be entirely dropped. It has so many possabilities to create side effects and nearly no safe usages. The enums should throw errors if serialized / unserialized. Maybe there could be a serializable enum class, which isn't comparable via reference.

How to reproduce

From 7899614f0dc543e3ad2f3f2304d2ed257d7bc7b6 Mon Sep 17 00:00:00 2001
From: Robert Worgul <robert.worgul@scitotec.de>
Date: Fri, 4 Nov 2016 15:10:33 +0100
Subject: [PATCH] test unserialize works multiple times

---
 tests/MabeEnumTest/EnumSerializableTraitTest.php | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/MabeEnumTest/EnumSerializableTraitTest.php b/tests/MabeEnumTest/EnumSerializableTraitTest.php
index 65e6280..01ce977 100644
--- a/tests/MabeEnumTest/EnumSerializableTraitTest.php
+++ b/tests/MabeEnumTest/EnumSerializableTraitTest.php
@@ -49,6 +49,9 @@ class EnumSerializableTraitTest extends TestCase

         // check if it's the same instance
         $this->assertSame($enum, $unserialized);
+
+        $unserialized2 = unserialize($serialized);
+        $this->assertSame($enum, $unserialized2);
     }

     public function testUnserializeThrowsRuntimeExceptionOnUnknownValue()
--
2.7.4

[prolic]

Didn't had a look into it so far, but this looks really bad! With enums being singletons instead of pure value objects (which compare equality by value, not by object identity) I definitely need serialization-support.

[delbertooo]

My tought for a serializable enum class was some kind of equals method, like Java. This class shouldn't use the singleton approach at all. If comparing via reference works some time, some users may use it, even though it's not safe.

[marc-mabe]

@delbertooo Your reported issue is a known issue also documented in the README.md

This is the reason why serialization support is not available by default but. Because serialization is often needed I have added the EnumSerializableTrait as a way to simply enable this feature. Of course it still has the known issue but it reduces it as best as possible.

The root issue for full serialization support is because Serializable::unserialize() as well as __wakeup doesn't allow to return an instance. Instead PHP will create a new instance of the class internally and calls one of these methods to fill up the new created instance - similar to how __construct works.

The only I could think of are:

• Throw an exception on trying to unserialize an already existing enumerator instance
• Change Enum::is() to return true in cases the compared enumerator is the same but different instance:

    final public function is($enumerator)
    {
        return $this === $enumerator || $this->value === $enumerator
            || ($enumerator instanceof static && get_class($enumerator) == get_called_class() && $enumerator->value === $this->value);
    }

[delbertooo]

@marc-mabe Thats not exactly the case reported in the readme. Everything works well if you unserialize this enum instance first. You can't return an instance in ::unserialize(), therefore your "magic" adds the instance (created by php) as the current instance for this enum value. And exactly this behavior has a huge disadvantage: if unserialize is called a second time in script execution, the enum value instance is already set to the last unserialized instance. At this point you can only ignore the new instance or replace the existing one. In both cases you can't compare every existing enum instance for this value via reference because there is more than one.

Imagine iterating over an array of serialized users where each user has a favorite direction. You unserialize one user per iteration. No favorite direction of any user will match the reference of the favorite direction of any other user - even if they are all the same direction.

The only edge case to this is: imagine the user having two directions: firstDirection and secondDirection. In one instance both would point to NORTH. If you unserialize this user, php would call unserialize for our enum value instance only once and they would be the same reference. But this is neither reliable nor controllable. IMO this is a big source for side effects, causing nightmare bugs where you debug hours to find the reason for something that "usually works".

[marc-mabe]

@delbertooo Still I don't know what you are asking for. You understand the root issue that it's not possible to unserialize an enumeration and compare it by reference in all cases.

Also that is the reason why serialization is not build-in by default - use the trait at your own risk. If you want to update the documented warning - please create a PR.

Additionally I added a code snipped in my last comment to support comparing by value instead of reference - did you tried that?

$activeStatus = UserStatus::ACTIVE();
$activeStatus2 = unserialize(serialize($activeStatus));
var_dump($activeStatus == $activeStatus2);
var_dump($activeStatus->is($activeStatus2));

Cheers Marc

[delbertooo]

@marc-mabe I tried to discuss the use of a built in functionality (serialization support) in a widely used library that has - at least in my opinion - more pitfalls than benefits. These are heavy side effects which are not obvious at all. Even if these are documented it just doesn't feel right. If I include a library which is widely used I expect it's built in features to be mostly intuitive and stable - and in this case: deterministic.

We should aspire something less prone to errors. It's like your default non-serializable enum. This enum throws errors if serialized. You can't run into side effects by accident. These is the kind of quality I would expect from a serializable enum, too.

The question is: how could we achieve that? We are limited through PHPs features. In my opinion it would be better to generally drop the reference equality for serializable enums. It would be a documented "feature" that a serializable enum can never be compared to an enum or serializable enum via == or ===. Each serializable enum would be a new instance. Therefore a user can never run in that kind of scenario where the comparison sometimes works, and sometimes not. This isn't as comfortable but much more intuitive to work with.

[marc-mabe]

@delbertooo First of all this library adds virtual enumeration support to PHP because PHP doesn't have it as a real build-in feature. That of course means that this library is limited.

We are trying your best to make it as close to real enumerations as possible!

My tought for a serializable enum class was some kind of equals method, like Java

This is something we have already. It's called Enum::is() and I added a PR to accept two different enumerator instances of the exact same thing. see #77

Interestingly also in Java enumerations are singletons. I'm not a Java developer but because of this the equal operator should work in Java, too. I have no idea if Java would have the same issue if you serialize and unserialize an enumerations.

In my opinion it would be better to generally drop the reference equality for serializable enums. It would be a documented "feature" that a serializable enum can never be compared to an enum or serializable enum via == or ===. Each serializable enum would be a new instance.

It doesn't make sense to change the general singleton behavior only because it's implementing Serializable interface. That would mean your question is for removing the singleton in general where you get a big no from me.

Also not injecting an unserialized enumeration in general does not make sense to me because later in your code has absolutely no logic about where objects are coming from. That's the reason why I try to reduce the behavior of having two different instances of the same enumerators as much as I can. In this case it's only possible once but it's still better then ńever.

And again with #77 you can surely use Enum::is() to check for identical enumerators safely.

[marc-mabe]

I'm closing this know as #77 is merged and there is nothing else I can do or like to change based on the above discussion.

@delbertooo please use the method Enum::is() which should solve your issue with the next minor version.

Thanks.