eloquent / enumeration

An enumeration implementation for PHP.
MIT License
147 stars 8 forks source link

__toString() should return value, not key. #19

Closed Bilge closed 9 years ago

Bilge commented 10 years ago

I believe __toString() should return the value of a constant, not its key, in the same way that calling Class::CONSTANT returns the value of CONSTANT, not "CONSTANT" itself.

If implementors really want to return the key name they can just set the value to a string matching the key name.

For my purposes I have created a new base class that overrides __toString() but it would be nicer if this was implemented in AbstractValueMultiton.

ezzatron commented 10 years ago

I could go either way on this one. One the one hand, yes, it makes some sense for multitons with values. On the other hand, not all multitons have values, so it would make the behaviour of __toString() inconsistent.

In any case, it would be a BC break, which would require a new major version. At this stage, stability of the API is more important to me, so it's unlikely to happen any time soon.

Bilge commented 10 years ago

The behaviour is only inconsistent among different implementations but different implementations have different use cases that are unrelated so I do not consider that an issue.

However, if you're scared of BC breaks then I suppose nothing will ever get changed.

ezzatron commented 10 years ago

It's not about being 'scared', it's about having a stable API.

marc-mabe commented 9 years ago

see marc-mabe/php-enum#24 & marc-mabe/php-enum#33

ezzatron commented 9 years ago

Thanks @marc-mabe, after reading through similar issues in your library, I'm pretty firmly of the opinion that __toString() should definitely return the key, and not the value.

@Bilge: Please see this comment for a summary of reasons why. Most importantly, __toString() must return a string, and enum values can be of any type.

Bilge commented 9 years ago

@ezzatron

return "$valueThatIsNotCurrentlyString";
ezzatron commented 9 years ago

That will explode if the value cannot be implicitly converted to string, like an array. Further, it's a fatal error to throw an exception inside __toString(), which will prevent any error conditions arising from type casting from being handled gracefully.

Bilge commented 9 years ago

Arrays are not allowed as constants.

ezzatron commented 9 years ago

Perhaps in C++, but this library supports Java-style enumerations (Multitons), which can have anything as a value.

ezzatron commented 9 years ago

Hmm, the docblocks do say scalar, but I'm pretty sure that's just an oversight on my part. Let me investigate.

ezzatron commented 9 years ago

To confirm, AbstractValueMultiton allows anything as a value, the scalar type hints are a leftover from when it was refactored out of AbstractEnumeration. So, to clarify, it's only classes that extend AbstractEnumeration that define their member values through constants:

use Eloquent\Enumeration\AbstractEnumeration;

final class Enumeration extends AbstractEnumeration
{
    const ONE = 1;
    const TWO = 2;
    const THREE = 3;
}

whereas classes that extend AbstractValueMultiton define their member values like so:

use Eloquent\Enumeration\AbstractValueMultiton;

final class ValueMultiton extends AbstractValueMultiton
{
    protected static function initializeMembers()
    {
        new static('ONE', 1);
        new static('TWO', array('two', 'two'));
        new static('THREE', new ThreeObject);
    }
}
Bilge commented 9 years ago

OK.