marc-mabe / php-enum

Simple and fast implementation of enumerations with native PHP
BSD 3-Clause "New" or "Revised" License
464 stars 36 forks source link

Allow operating with `private const` #142

Closed Ocramius closed 4 years ago

Ocramius commented 4 years ago

private const should probably be allowed in the context of enum implementations, to prevent third-party implementations from coupling with the constant structure/definition.

Detailed description:

Tried today:

class MyEnum extends Enum
{
    private const foo = 'bar';
}

var_dump(MyEnum::byName('foo'));

Crashes with

InvalidArgumentException: MyEnum::foo not defined

This is caused by the defined() function call in:

    final public static function byName(string $name)
    {
        if (isset(self::$instances[static::class][$name])) {
            return self::$instances[static::class][$name];
        }

        $const = static::class . "::{$name}";
        if (!\defined($const)) {
            throw new InvalidArgumentException("{$const} not defined");
        }

        assert(
            self::noAmbiguousValues(static::getConstants()),
            'Ambiguous enumerator values detected for ' . static::class
        );

        return self::$instances[static::class][$name] = new static(\constant($const));
    }

Ideally, MyEnum::byName('foo') should resolve normally.

marc-mabe commented 4 years ago

Thanks for reporting!

I don't think supporting private const would be correct here. That this enumeration impl. is based on constants is just a workaround for the lack of native enum in PHP. If you think of native enumeration an enumerator available for it's enumeration doesn't make sense to me.

What we can do is changing the error to be more meaningful in this case.

Ocramius commented 4 years ago

Effectively, all I care for are the named ctors provided by this library, the instance methods, and the restrictions imposed (singleton-ish instances, serialization, etc), while the constants are not part of the API of any of these enumerable types.

marc-mabe commented 4 years ago

I'm not sure I understand correctly. Please could you make an example or explain in more detail what you are trying to do.

Ocramius commented 4 years ago

It really is quite simple:

/**
 * @method static self lax()
 * @method static self fra()
 */
class FlightDestinations extends Enum
{
    private const LAX = 'LAX';
    private const FRA = 'FRA';
}

 // legal: 
$destination = FlightDestinations::lax();
// illegal: indicates that consumer/developer is trying to push around strings,
//          whilst we should keep `FlightDestinations` as object as much as possible
$destination = FlightDestinations::LAX;

The fix is quite simple BTW: instead of using a \defined() call, we can check the class constants via reflection (or by looking into the cache, after it is populated).

I'm aware that marking them as private won't disallow ->getValue() calls, but I'd still wish to remove this one use-case that will come from auto-completion by less experienced devs :+1:

marc-mabe commented 4 years ago

I see where you are coming from now. But I still don't think this would be a good idea and it would also be a BC break.

E.g.

Some use a private const to define a default enumerator and add a getter for that.

I sometimes use private constants where I map enumerators to something else as long it's logical related.

Ocramius commented 4 years ago

I see: indeed think about the BC break (and just noticed that the constants are filtered for being public to be considered enumerators).

I think I will have to live with the limitation or design my own implementation then, thanks!