Compatibility with php-ds/ext-ds

[jacek-foremski]

Hello.
I wanted to use this package for my project (BTW good job, it looks very useful).
However I encountered an issue when I tried using it alongside php-ds/ext-ds (https://github.com/php-ds/ext-ds). What I would like to do is to use Enum objects inside Set structure, which by definition should store only unique values. The objects are compared using Hashable interface (https://github.com/php-ds/polyfill/blob/master/src/Hashable.php).
The issue is that my enum should therefore extend your Enum abstract class and implement Hashable at the same time. Unfortunately both have "equals" method declared and there are incompatibilities between the two. Due to how the Enum abstract class is designed, I cannot solve it without redeclaring the abstract enum class completely. The issues are:

• "equals" method from this package accepts only Enums as an argument. I think here you could safely drop the type declaration without it mattering much (you can check the type inside). Also the argument could be made that method like this should just return false when different type is passed instead of throwing an exception.
• "equals" method from php-ds/ext-ds has return type declaration declared (bool), while method in this package has none. I understand that the difference comes from the fact that you want to keep older PHP compatibility, so as long as you continue to do that there is not much we can do about it. Return type declaration can also be added by overriding the method, however in this case the abstract Enum class declares this method as final. While I agree with the reason why this was done, it also makes it impossible for me to do anything about that issue.

I'll probably use my own Enum class for now, but I would like to hear if you would be interested in fixing that issue somehow. I would be glad to help if you do.

[mnapoli]

Hi, from what I understand here is what you suggest:

• require PHP 7.0 and add return types
• change equals() to accept anything (and return false when it's not an enum)

Those sound like very sensible changes. Did I miss anything?

[jacek-foremski]

That is correct. I'm glad to hear that you feel positive about the changes.
An alternative to requiring PHP 7 and adding the return type is to remove the "final" keyword (which could be done without the BC Break). It would allow to override the method and add the return type in the override. However I feel that adding the return type in the project would be better option in the long term.

Do you want me to open PRs for these changes?

[mnapoli]

Targeting PHP 7 is definitely a good move honestly, it is time :)

[mnapoli]

Do you want me to open PRs for these changes?

Sorry I missed that, a PR would be welcome indeed.

[jacek-foremski]

Great. I'll do that next week and let you know.

[willemstuursma]

What about throwing an exception is it is not an enum. This will make sure implementation errors do not go unnoticed.

[jacek-foremski]

It's of course an option, but I would disagree that passing an object of different type is an implementation error.
There might be a valid reason to compare objects, even if they are not of the same type.
Imagine having the collection of Hashable objects. Also imagine that my custom enum extends the enum from this package and also implements Hashable interface. Now I want to put inside the collection every Hashable object, not only enum. If "equals" would throw an exception if types are different, then I would have to specifically check if the collection elements are Enums and only then call this method (or catch an exception, but you get the idea). This situation is not an implementation error, but a valid usage - objects in collection don't have to always be the same type.

[mnapoli]

@jacek-foremski agreed, having a equals method that can take anything is a common pattern in value objects (at least from my experience).

[jacek-foremski]

As discussed, I created two PRs: #87, #88

[mnapoli]

Was closed automatically by the merge ^^