Skip to content
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

Constant value can be a array #53

Open
peter-gribanov opened this issue May 16, 2017 · 22 comments
Open

Constant value can be a array #53

peter-gribanov opened this issue May 16, 2017 · 22 comments

Comments

@peter-gribanov
Copy link
Contributor

peter-gribanov commented May 16, 2017

Bug at this line. (proof)
It will be better use the name of constant for __toString().

@mnapoli
Copy link
Member

mnapoli commented May 16, 2017

Hi, I'm not sure I understand could you explain a bit more?

@peter-gribanov
Copy link
Contributor Author

peter-gribanov commented May 16, 2017

This is an exaggerated example:

class Foo extends Enum
{
    const VIEW = ['foo', 'bar'];
    const EDIT = [1, 2, 3];
}

bug:

echo Action::VIEW(); // PHP Notice: Array to string conversion

@peter-gribanov
Copy link
Contributor Author

It will be better use the name of constant:

echo Action::VIEW(); // VIEW

@mnapoli
Copy link
Member

mnapoli commented May 20, 2017

OK I see, we cannot change the existing behavior it would be a BC break.

However we could check if the value is an array: in that case there could be, for example, an implode().

@chippyash
Copy link

Not sure it's a bug. If people would use it for arrays instead of a single value, then the behaviour could be considered unpredictable, as you now effectively have a set and not an enum;

@peter-gribanov
Copy link
Contributor Author

@mnapoli If a multidimensional array is used, implode() does not solve the problem. In addition, it will not be very readable.

I advise simply to replace the transformation of the object to a string.

public function __toString()
{
    return $this->getKey();
}

@chippyash you are right, but i think that we should not rule out such an opportunity.

@mnapoli
Copy link
Member

mnapoli commented May 22, 2017

@chippyash yes that's a good point of view.

@peter-gribanov as I said this solution is a BC break, we cannot do that.

__toString() is usually used for debugging so it would make sense to me to dump something useful.

Here are our options:

  1. do nothing and keep the error
  2. dump something useful
  3. ?

@chippyash
Copy link

chippyash commented May 22, 2017

@mnapoli

3rd option is just to document it. i.e. don't try and use enums as sets, the behaviour is unpredictable.

4th option, reinforce 3rd option and put in a check that ensures that enum values are scalar and not traversable. Throw an exception if contravening. This might be considered BC breakage however. But worth putting on the list for the next version maybe.

@mnapoli
Copy link
Member

mnapoli commented May 22, 2017

I see nothing wrong with using an array as a value though 🤔

@chippyash
Copy link

Consider this as a traditional explanation of the difference between ENUM and SET as applied to SQL (where they get used a lot,)

An ENUM value must be one of those listed in the column definition, or the internal numeric equivalent thereof. The value cannot be the error value (that is, 0 or the empty string). For a column defined as ENUM('a','b','c'), values such as '', 'd', or 'ax' are illegal and are rejected.

A SET value must be the empty string or a value consisting only of the values listed in the column definition separated by commas. For a column defined as SET('a','b','c'), values such as 'd' or 'a,b,c,d' are illegal and are rejected.

Therefore if you want to construct a SET of ENUMs, use an array as a simple collection, or better still some immutable type. (see https://github.com/chippyash/Monad and the Monadic Collection as an example. Other Collections exist. NB. The Monadic collection checks on type, not value, so you'd have to add some additional functionality to ensue set membership wasn't broke. That'd be the same for any set implementation.)

@mnapoli
Copy link
Member

mnapoli commented May 22, 2017

Thanks for the explanation, personally I'm still not really convinced we must apply the same logic to PHP.

@chippyash
Copy link

@mnapoli

I'm still not really convinced we must apply the same logic to PHP.

Why not? In any language (programming or linguistic,) there are some constants. We are discussing a data structure type which is universal across programming languages. To say that we shouldn't do the same in PHP would lead to confusion. The Enum lib does what it says it will do on the tin. We can always pervert PHP code to do something it's not intended to do. Ever seen a routine to morph one class into another? Not supported officially, but PHP let's you do it. Doesn't make it right though.

Enum was intended to house scalars, and provide a much needed basic type implementation. That's what most people would expect it to do. Why break with expectations? As it is, the library is clean and simple with a single concern. Perfect imho.

@mnapoli
Copy link
Member

mnapoli commented May 22, 2017

The only problem we are facing is __toString() it seems, so it doesn't seem like a good reason to prevent that kind of usage. Let's see if other problems pop up because of arrays.

@chippyash
Copy link

What about nested arrays? Or arrays of objects? Or even just objects? Or resources? How do you come up with a universal __toString() method for all of those? Have a look at https://github.com/chippyash/Monad/blob/master/src/chippyash/Monad/Match.php to see the pain you have to go through to be 'generalist'. But once you start down the route, you have to cover them all.

Any way. Good discussion, I'm sure you'll do what you think is right. Thanks for the lib so far.

A

@mnapoli
Copy link
Member

mnapoli commented May 23, 2017

You cannot add objects or resources into a constant though.

@peter-gribanov
Copy link
Contributor Author

@mnapoli No. You can define constants as a resource

http://php.net/manual/en/language.constants.syntax.php

It is possible to define constants as a resource, but it should be avoided, as it can cause unexpected results.

@peter-gribanov
Copy link
Contributor Author

I did not try to do it. Most likely this refers to the function define()

@chippyash
Copy link

Anyway - here is an example of how you might do sets by combining a collection and enum

@mnapoli
Copy link
Member

mnapoli commented May 24, 2017

@peter-gribanov this doesn't apply to class constants.

@peter-gribanov
Copy link
Contributor Author

@mnapoli Yes. I made a mistake. This only works for global constants.

define('FILE_RESOURCE', fopen(__FILE__, 'r'));
var_dump(FILE_RESOURCE);

print

resource(22) of type (stream)

But you can still use multidimensional arrays as a constant value:

class Foo
{
    const DATA = [
        [
            [
                [
                    'foo' => 'bar',
                ],
            ],
        ],
    ];
}

@peter-gribanov
Copy link
Contributor Author

__toString() is usually used for debugging so it would make sense to me to dump something useful.

@mnapoli you use for debugging, and i use to display a readable value on the screen.

class Foo extends Enum
{
    const VIEW = 1;
    const EDIT = 2;

    public function __toString()
    {
        return 'acme.demo.foo.'.strtolower($this->getKey());
    }
}

Use translate value in Twig

Foo: {{ foo | trans }}

@mnapoli
Copy link
Member

mnapoli commented May 24, 2017

@peter-gribanov that would still work since you are overriding the method.

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

3 participants