Skip to content

Base method toArray processes all constants, including privates. #62

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

Closed
marrch-caat opened this issue Jan 9, 2018 · 7 comments
Closed

Comments

@marrch-caat
Copy link

marrch-caat commented Jan 9, 2018

Right now, Enum works fine for simple classes, but when I want to add something custom and add some private constants to a class, they are treated as if they were public: they are listed in toArray() output and thus their values make isValid() return TRUE, their names produce TRUE on isValidKey() etc etc. It is very much wrong in many cases.
So I propose the following change:

    public static function toArray()
    {
        $class = get_called_class();
        if (!array_key_exists($class, self::$cache)) {
            $reflection = new \ReflectionClass($class);
-            self::$cache[$class] = $reflection->getConstants();
+            $consts = $reflection->getConstants();
+            self::$cache[$class] = array_filter(
+                $consts,
+                function($v, $k) use($reflection) {
+                    return $reflection->getReflectionConstant($k)->isPublic();
+                }
+            );
        }

        return self::$cache[$class];
    }

@mnapoli
Copy link
Member

mnapoli commented Jan 9, 2018

I think that this is a good thing that private constants are recognized by the enum, that forces the user to use the methods instead of the constants (which is a common mistake).

Example:

class Action extends Enum
{
    const VIEW = 'view';
    const EDIT = 'edit';
}

doSomething(Action::VIEW); // ERROR: not an instance of Action
doSomething(Action::VIEW()); // OK

If the constants are private then autocompletion will show only ::VIEW() which is better!

@chorry
Copy link

chorry commented Jan 10, 2018

I think its not about using constants vs methods, its about ::values giving out all possible constants:

class Action extends Enum
{
    const VIEW = 'view';
    private const HIDDEN = 'youShouldNeverSeeMe';
}

Yes, autocomplete won't show you HIDDEN, but Action::values() will return VIEW and HIDDEN as well. So the question is this behaviour intended by design, or actually a bug?

@mnapoli
Copy link
Member

mnapoli commented Jan 10, 2018

This is not a feature, it's by design (it's a good thing):

class Action extends Enum
{
    private const VIEW = 'view';
}

doSomething(Action::VIEW()); // OK

Is that what you were asking?

Said another way, private constants are part of the enum.

@mnapoli
Copy link
Member

mnapoli commented Jan 10, 2018

I've opened #63 to address this confusion.

@marrch-caat
Copy link
Author

So looks like there is no proper way to do something like

class SMSType extends Enum
{
    const TYPE1 = 'type1';
    const TYPE2 = 'type2';

    private const TYPE_LABELS = [
        self::TYPE1 => 'Label 1 text',
        self::TYPE2 => 'Label 2 text',
    ];

    public function getLabel() {
        return static::TYPE_LABELS[$this->getValue()];
    }
}

(Of course, I can always replace private const by a private static field, but I can't treat that as a proper way to do, it's just a not-so-bad workaround for the Enum class limitations).

@marrch-caat
Copy link
Author

And BTW, "problem" with using constant instead of method and vice versa is minimized (in fact, it just doesn't appear) by using type hints and PHPDoc notations.

@holtkamp
Copy link
Contributor

holtkamp commented May 7, 2018

I kind of agree with #62 (comment) and disagree with #63

In case you want the constants to be used by other parts of code => make them public. In case someone wants to use constants for internal use (for whatever reason, one example is #62 (comment)), then use private visibility. That is the whole idea of 'visibility', right? Why expose these private/internal values to the outside world?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants