#Fix: Php enum support by charjr · Pull R...

1 messages · Page 1 of 1 (latest)

cinder ravine
#

Any help on this would be greatly appreciated. Thank you 🙂

tulip turret
#

I'll take a look at this soon.

tulip turret
#

Code seems ok, but if you send a trace it would be more helpful.

#

Do you have Dagger cloud set up, @cinder ravine?

tulip turret
#

Looking at the PR there's a few things that's important to know. Enum values need to be "names". Basically think about what's allowed as a programming variable name, not the name of an enum member. That means they should only be strings, that begin with a letter, but can have numbers and underscores, and by convention should be in SCREAMING_SNAKE_CASE. The SDK is allowed to create enum names based on the conversion of that value to the language's naming convention. So you're expected to support this:

enum Dummy: string {
    case Hello = 'HELLO';
    case World = 'WORLD';

But not this (comma, space, exclamation point):

enum Dummy: string {
    case Hello = 'hello, ';
    case World = 'world!';

The first one will produce the following GraphQL schema:

enum Dummy type {
    HELLO
    WORLD
}

As you can see, GraphQL enums don't have names+values, they're considered values that are valid names (as in identifier or variable names).

So the second example would produce a syntax error:

enum Dummy type {
    hello,
    world!
}

Same goes for other types that aren't strings, like integers (can't begin with a number). This will probably be improved in https://github.com/dagger/dagger/issues/8671.

#

As for scalars, here's the thing. If you have a SCALAR_KIND, that means it's a custom scalar, and it's always a string. Modules don't support custom scalars yet, they're only defined in the core API. dagger.JSON and dagger.Void are examples of that. This may be confusing since there are other kinds of scalars, although for a given type you only have one of these:

  • BOOLEAN_KIND: built-in scalar of type boolean
  • INTEGER_KIND: built-in scalar of type integer
  • STRING_KIND: built-in scalar of type string
  • ENUM_KIND: an enum type with a name and an enumerations of string based scalars (their values)
  • SCALAR_KIND: a custom scalar with a name; all custom scalars are string based, but they're implemented via two functions to serialize and deserialize off of the string value
  • VOID_KIND: a Dagger specific custom scalar that's always an empty string
#

In the general sense, scalars are leaf values in a GraphQL type system.

cinder ravine
#

... you're expected to support this:

enum Dummy: string {
    case Hello = 'HELLO';
    case World = 'WORLD';

But not this (comma, space, exclamation point):

enum Dummy: string {
    case Hello = 'hello, ';
    case World = 'world!';

When serialising an Enum I've accounted for this by serialising the name instead of the value.
So for both examples it serialise the names: Hello and World.
In the second example I purposefully made the values hello, and world! to assert that the values do not get serialised.

Similarly if a user must give an enum as an argument, it expects the name. So you would be expected to send in dagger call -m mod func --enum-arg=Hello

I figured this made for the best user experience for a PHP developer as:

  • It doesn't impose any additional rules they aren't already used to (In PHP, enum names have to follow similar conventions; they MUST start with a letter, but may contain numbers and underscores)
  • It provides a consistent interface for all types of PHP Enums:
enum Dummy
{
    case North;
    case East;
    case South;
    case West;
}
enum Dummy: string
{
    case North = 'north?';
    case East = 'EaSt';
    case South = 'SOUTH';
    case West = 'West!!!!';
}
enum Dummy: int
{
    case North = 0;
    case East = 1;
    case South = 2;
    case West = 3;
}

All 3 of the above are valid PHP Enums. But by basing it off the name they will ALL serialise to North, East, South or West

#

they should only be strings, that begin with a letter, but can have numbers and underscores, and by convention should be in SCREAMING_SNAKE_CASE.
Do I need to convert enum names to SCREAMING_SNAKE_CASE for them to be recognised?

cinder ravine
tulip turret
tulip turret
cinder ravine
tulip turret
cinder ravine
#

My naive assumption here is that it doesn't seem to be registering the values at all

ashen peak
tulip turret
ashen peak
#

so php has two types of enum, unit enums and backed enums; the second type allows for a named case to have a scalar representation

cinder ravine
#

but both types of enums always have names, and names are always a string

#

so we can rely on that

ashen peak
#

graphql as far as i see only has an enumeration which defines a set of possible values

#

the name of the case is the most logical thing to use here as otherwise you'd have to not support one of the two enum types

tulip turret
#

Look, if you do this:

enum Dummy: string
{
    case North = 'north?';
    case East = 'EaSt';
    case South = 'SOUTH';
    case West = 'West!!!!';
}

This will produce the following in a Python module that uses the PHP module as a dependency:

class Dummy(enum.Enum):
  NORTH = "NORTH"
  EAST = "EAST"
  SOUTH = "SOUTH"
  WEST = "WEST"

So you have to explain that enum values in PHP only exist on the module that defines it. Other modules won't see it. The most appropriate thing for us, since GraphQL only has values in enums, to use the values in the languages, and have the SDK generate names that match the values but have their case converted to the proper naming convention, on the client code. This follows that module authors should define enums with names that match their (string based) values, and SDKs should either enforce, document or warn about this.

Having said that, @placid fractal has recently added support for custom GraphQL directives in our internal code for the GraphQL server so it's now possible to produce this schema:

enum Dummy {
    HELLO @value("hello, ")
    WORLD @value("world!")
}

As I pointed out in #1290721727071916076 message, the improvement for this is in https://github.com/dagger/dagger/issues/8671, where we then allow SDKs to register both a name and a value, and update codegen in SDKs to produce the right names and values for enums in their languages.

What do you want to do? Have PHP work differently than the other SDKs and explain the difference to PHP users until this change becomes available? Bring the PHP SDK inline with the others both now and then? Or wait for that to release enums in PHP?

placid fractal
#

fyi custom directives aren't yet merged

cinder ravine
#

I think we should wait till that PR is merged so that we can support all enums without breaking backwards compatibility

placid fractal
#

(should be good, just need reviews)

cinder ravine
#

However we still can't seem to register enum values at all

#

I just tried hardcoding in a case of HI on any enum it finds at all.

                $enumTypeDef = dag()
                    ->typeDef()
                    ->withEnum(
                        $daggerObject->getNormalisedName(),
                        $daggerObject->getDescription(),
                    )->withEnumValue('HI', 'hello');

Even with this, the trace does not show any values being registered to the enum: https://dagger.cloud/charjr/traces/c1413fce3b5065d3047509c82164c4d7

cinder ravine
placid fractal
cinder ravine
#

The trace shows the call to WithEnum, but it does not show any WithEnumValue.

tulip turret
cinder ravine
tulip turret
#

Do you have a simple example I can try?

#

To run locally

cinder ravine
#

It registers the enum; If I call it without passing the argument, it works.

If I try passing the argument however, I get an error because it doesn't recognise the value and actually has no values at all

#

bear in mind it's based off of my fix-php-enums branch

cinder ravine
#

I'm sorry if I wasted your time.

#

I had two similarly named branches and hadn't noticed I was testing the wrong one.

It does work now

#

I will rebase it and get it ready. But we will hold off until Jed's PR is merged so we can use the current implementation.

tulip turret
cinder ravine
# tulip turret Ok, but to be clear, the PR that's ready for review is just for adding support f...

I think that should be fine.

If you don't mind me asking, could you explain what issue this causes:

This will produce the following in a Python module that uses the PHP module as a dependency:
class Dummy(enum.Enum):
NORTH = "NORTH"
EAST = "EAST"
SOUTH = "SOUTH"
WEST = "WEST"

I understand they wont have access to the value of the enum, but if all they're doing is passing the value back to the PHP SDK, what issue would it cause? As long as the PHP module knows how to get the original enum from it?

#

I only ask in case I am missing something, so I can better understand the interaction between modules

tulip turret
#

That's only an issue if the value is actually needed outside the module that defines it. Depends on the use case, and I'd actually also like to know more about those use cases. It can also be confusing to a user of the dependency, to see different code in the defining module than on the client one (this has happened with Go users).

For example, if a module foo defines this enum:

enum Bar: string
{
    case North = 'north?';
    case East = 'EaSt';
    case South = 'SOUTH';
    case West = 'West!!!!';
}

Another PHP module using it as a dependency will generate this client binding:

enum FooBar: string
{
    case North = 'North';
    case East = 'East';
    case South = 'South';
    case West = 'West';
}